コードレビューをする際と、してもらう際に気をつけるべきだと思っているをまとめておきます。
レビュイーとして大切なこと
コードレビューをお願いする前に
レビュアーが高いパフォーマンスを発揮するためには、レビューを受ける人の心構えと事前の打ち合わせが実は最も大切です。
特に大きめの変更のコードレビューをお願いする前にすると良いこととしては、
- まず、レビュアーを確保する
- そのレビュアーと大まかな設計の合意をとる
という方針でやっていくのが良いです。
レビューしていても、根本をひっくり返すような指摘はしにくいですし、したとしても、それなら1から書いたほうがはやくて良い物が出来るといったことが簡単に起きます。
「レビュー後に直せるもの < レビュー前に直せるもの」
ということを意識して、Issueなどを用いて、出来る限り事前に設計、打ち合わせをしましょう。そうすればレビュアーもすんなりレビューできるはずです。
もちろん数行の小さい変更や、簡単なBug Fixまで全てこうする必要があるわけではありません。
レビューのリクエストの際に
ここでは、コードを書いてPull Requestを送る際に意識しておくと良いことを書いておきます。
- できるだけ小さい単位でPull Requestを送る
- 何故やったかを書く
- テストを書く
できるだけ小さい単位でPull Requestを送る
そもそも論ですが、例えば、変更が1000行を超えるようなものをしっかりレビューするのなんてほぼ不可能です。
できるだけ、細かく機能を分け、小さい単位でRequestを送るようにしましょう。
gitの場合、masterや開発の本流ブランチに直接マージしては困るものも、トピックブランチを作ってそのトピックブランチへのMergeリクエストを送るようにすればこれが可能です。
細かくRequestを送ることが難しい場合は、早めにPull Requestを作成し、随時見てもらうという方法があります。[WIP](Work in Progressの略)をタイトルの頭につけて、完成前のある程度中身が見えてきた段階でPull Requestを送るとよいです。
何故やったかを書く
何をやったかはcommitにも大体書かれますし、書かれていなくてもコードを読めばわかるはずですが、何故そのRequestが必要だったかは書かないとわかりません。
すごく自明で無い限り書くようにしましょう。対応する適切なIssueがある場合は、そのIssueに紐付ければそれで終わることもあります。
テストを書く
「レビューをしてもらってるわけだし、挙動があっているかはレビュアーに確認してもらおう」というような事は、程度の差はあれ思ってしまったりします。でも、出したコードにバグがあった時、レビュアーにももちろん責任はありますが、基本的には、やはりコードを書いた人の責任です。レビュアーに確認してもらいたいような部分は積極的にテストを書くようにしましょう。レビュアーもテストのレビューのほうがしやすいです。
特に、モデルに相当する部分の単体テスト、サービスのコアとなるような重要な機能のリクエストテストは書くようにすると良いです。
レビュアーとして大切なこと
レビューする時
自分の経験上、レビュアーのレベルは、以下の 1.まで出来る人と、2.まで出来る人の大きく2つに大きく別れる気がしています。
- diffとして見えている問題の指摘ができる
- diffとして見えていない問題の指摘ができる
- 変更しようと思っていない部分への副作用の考慮
- 将来的に生じる問題の予測
1.の見えている問題としては、コーディング規約に従っていない、スペルミス/タイポ、名前がわかりにくい、if 文の条件が間違っているなどが挙げられます。
2.は、単純にRequestに現れていないコード部で起こる問題という"空間的"に見えていない部分と、将来の変更で起こる問題という"時間的"に見えていない部分、2つの考慮が出来る必要があります。前者はそのコードベースでの経験、後者はエンジニアとしての経験による部分が大きいですが、見えてない部分も意識することで1レベル上のレビューが出来ると思うので、是非、2.のレベルの指摘が出来るようになってください。
ぱっとできるポイントとしては、自分が実装するならどこを気をつけるかあたりをつけて読むこと、コードの不吉な匂いなど典型的な悪いパターンをある程度知っておくことがあります。
初心者によるレビュー
エンジニアの経験的にどのレベルになった人からレビューをやらせるかですが、Wantedlyでは、新卒であろうとインターンであろうと誰もがレビューをする方がいいという結論に至っています。もちろん最初のうちは初心者がレビューした後、より経験のあるエンジニアがレビューします。最初からレビューさせるメリットは以下です。
- 実は初心者ほどコードを読むことが仕事になった方がレベルが上がる
- 特にその後、経験者が別の問題を指摘してしまった時、最もレベルが上がる
- 実は初心者の方が丁寧にレビューするので、忙しい事が多い経験者エンジニアの負荷も軽減し、全体として効率が上がる
- あの人はレビューができるけど、あの人はレビューができないみたいな、あまり意味のない別け隔てがなくなり精神的にも良い
初心者でもできるレビュー
基本的には、変更部分に書いてある事の意味は完全に理解するように努めます。その時、
- 自分がよくわからない部分は質問的に聞いてみる
- よくわからないなと思った部分は大体コードも怪しいです。
- 読みにくい部分を指摘してあげるだけでも十分意味があります。
- 怪しい部分はテスト書いてという
- 今書かなきゃ一生書かないです。今が書かせどきです。
- 細かいスタイルの修正も、神経質と思われそうな恐怖を打ち崩してする
- gitなどで管理している場合他の人がスタイルの修正をするとその行周辺で問題が起こった時、誰に聞けばいいかがわかりにくくなってしまう。コーディングスタイルの修正はその場でその人がする方がいいです
- その時に、細かいことなのはわかってるけどみたいな意味で
nitとかつけてあげるといいかも
この中でも「質問的に聞く」という行為は、初心者じゃなくてもかなり大事です。実際バグを発見するのは聞かれた本人であることも多いです。どんどん質問しちゃいましょう。
気をつけて見るポイント
とはいえ、忙しかったりすると、1行1行みたいなのは辛かったりします。そういう時はやはり優先度がついた色眼鏡で見ます。
バグを生みやすいところを重点的に見る
- 境界条件でうまく動くか確認
- 特に要素がない時 (0, nil etc.) もエラーが出なく、表示的にも問題ないか
- テストがちゃんと問題が起こっているとき、きちんとFailするテストになっているか
- 名前の付け方が悪くて誤解を生みやすいものがないか
- 違う意味なのに、同じ名前で使われている変数はないか
- 結果を返し忘れたりしてしまうコードパスがないか
- 1つのメソッドにやたらとif文のネストある構成になっていないか
- if文のネストが多いと網羅性のあるテストも書きにくいので大抵設計を見なおしたほうが良いです
- バグを生みやすいコードがないか、脳内ストックから引き出す
- 例えば、rubyでif文以外では
saveでなくsave!を使っておく等 - 普段からそういうコードの脳内ストックを増やすようにしましょう！
バグや設計変更があった時に、大変になるところを重点的に見る
- モデルの修正は大変なのでビューに比べて念入りに見る
- メールの送信など送ってしまったら取り返しがつかないものはきちんと試してみる
- セキュリティの問題を抱えやすい場所は注意する
- ユーザー入力を埋め込んでる場所など (XSS/SQL Injection etc)
- サービスの根本的な価値や、お金につながるような変更は死ぬ気で見る
レビューの役割分担
組織が大きくなっていったり、それぞれが把握している部分が局所的になってくると、レビューを役割分担し２人以上でレビューを行ったほうが良いこともあります。
そういう時は、以下の3つの視点で役割分担してレビューを頼むとうまくいきやすいです。
3つのレビュー
- ロジックレビュー：やりたいことがきちんとコードとして実現できているか見る人
- レビューアー適任者: 同じプロジェクトをやっている人
- リーダビリティレビュー：コード規約、その言語らしい書き方ができているかを見る人
- レビューアー適任者: その言語が得意な人
- オーナーレビュー：そのコード入れてを他の部分を壊さないか、そもそも入れるべきかを判断する人
- レビューアー適任者: 変更を入れる部分のオーナー
もちろん、1.,2.,3.が同一人物であるというという場合もあるでしょうが、この仕組で運営すると、１人のレビュアーだった頃と全体の合意を取るのに時間がかかったりするので、各役割のレビュアーはそれ以外の部分に口を出さないという割り切りも大事かもしれません。
これ以外にも、それぞれ必要な場合、
- デザイン/UX的にOKか
- セキュリティ的にOKか
- 法的にOKか
などを判断する必要があることもあります。これをレビューと呼ぶのかは微妙なところですが。
レビューの最後に
特に、頑張りの見られるコードの後には、疲れを癒やすLGTMをあげましょう！
ちなみに自分は http://lgtm.in/ から探しましたが、結構自前で作るのが流行ってます。
