コードレビューするときの視点

普段、Ruby(RailsやSinatraといったWAFにおける)のコードレビューするときに、こういった事を念頭においてレビューする、というものが自分の中だけで閉じている気がしているので、文章にしてリスト化しておくことでコードレビュー時に、自分自身も漏れが少なくなり、相手にもレビュー箇所を伝えやすいんじゃないかと思案してみた。静的解析(Rubyであればrubocopやhoundなどを使う)して指摘できる部分は極力はぶいたものとなっています。

TODO: パッと思いついたことで網羅できている感じはないので、増やしていく。

アプリケーションが解決するドメインの視点

ドメインについてレビューすることが一番重要であると考えているので時間をかけるところだったりします。つまり、問題を解決する戦略についてレビューするところ。

解決しようとする問題が明確になっているか
- けっこうあったりする。たとえば、p-rにいろいろ含まれていたりするのがその傾向にあって、レビュイーの人が問題を明確になってなかったり
  - やんわり、イシュー分割しましょうと促す
解決する問題に対して粒度が適切かどうか
- めっちゃ頑張ってオーバースペック過ぎたり、逆にもうちょっと汎用的に作っておいたほうが良かったり

プログラミングという視点

プログラミング言語特有のお作法だったりするものがあると思うのだけど、この3つは必ず見ます。

条件文が明確かどうか
- if文やcase文の条件文が正しいかどうか
データ構造が適切かどうか
- それにともなうアルゴリズムは適切かどうか
各名前付け(クラス名、モジュール名、メソッド名、変数名)が見通しがよいか、妥当かどうか

Ruby固有の視点

Enumerableなオブジェクトに対する操作が冗長になっていないかどうか
- たとえば、Arrayの要素を足し合わせる時に、
  
  sum = 0 array.each { |v| sum += v }
  
  と書くより、
  
  array.inject(:+)
  
  と書いたほうが見通しがよくなる(と思っているのはinjectおじさんだからかも)
- 冗長かな、と思ったらEnumerableのメソッドをひと通り確認する

Rails(やSinatra)、各gem固有の視点

モデル(ARを使う場合)

N+1問題が起きていないかどうか
- includes を使うことでだいたいにおいて回避可能
LEFT OUTER JOINが起きていないかどうか
- 基本的にLEFT OUTER JOINを発行するくらいならクエリを分けたほうがロジックの見通しがよくなることが多い(NULLを含めるのは避けたい)
  - その結果N+1問題が発生するのは本末転倒なので、その場合、in句でしっかり1クエリで取れるようにする
scopeの使い方は妥当かどうか
- scope定義してみたはいいが1箇所しか使わない、という場合が多いことがよく見受けられる。意味のある名前にすることでロジックの見通しがよくなるなら構わないが、where句をその場で書いたほうが見通しは良くなることが往々にしてある。
>= のような演算の場合に、べたにSQLを書かないで、arel_table を使っているかどうか

コントローラ

たとえば、before_action 等で params に値を突っ込んでないか(個人的にジャグリング問題と名づけている)
トランザクション境界は明確になっているか

ビュー

ロジックを書きすぎていないか
- helperに分離する

テスト(RSpec + FactoryGirl + RSpec mocksを使う場合)

letとlet!は使い分けているかどうか
mockとstubは使い分けているかどうか
FactoryGirlでテストデータを定義するときにPK(特に、サロゲートキーの場合)を固定の値にしていないかどうか

gem

メンテナンスされていないgemを採用してないかどうか
- 採用を考えている時点で最後のコミットが1年前くらいだとプロダクトに使うものとしては警鐘を出す
  - どうしてもそのgemを使わないと、問題を解決するのに遠回りになったりする場合、そのgemのメンテナになる意気込みで採用する