ただし、以下は前提。
* Github、あるいはそれに類似したレビューシステムを利用している。SCMはGit。
* 商用というか、仕事のコードレビューである。
プルリクの目的が明確であり、レビュワーがそれを理解できるようになっている
そのプルリクが何をしようとしているのか、レビュワーはレビュー前に理解していないといけない。だからプルリクの最初のコメントでそれがきちんと説明されているかを確認する。目的がわかればレビューにどれだけ力をかければ良いかもわかる。例えば一回だけしか使わない運用スクリプトなら、細かいフォーマットの崩れとかを逐一指摘する必要はないが、実行内容の正しさは十分注意してみないといけない。
差分を見ればそのプルリクで何をやっているかわかる、という主張ももしかしたらあるかもしれないが、それはちょっとやろうとしていることが逆転している。目的に対してコードの修正が正しいことを確認しなければいけないのに、書かれたことからやろうとしていることを忖度しようとしたら、本来の目的でないものをOKとしてしまうかもしれない。
Githubのプルリク画面だけではなく、checkoutして全体のコードを見て、修正漏れがないことを確認する
当たり前といえば当たり前だが、ある程度の修正をちゃんとレビューするならプルリクの画面だけでは仕切れない。一旦ローカルにチェックアウトして全体をレビューする必要がある。変更する必要があるのに漏れているケースなどは差分には出てこない。修正漏れがないことを確認する必要がある。チェックアウトした上でのgrepはとても効果がある。うまくキーワードを見つけてgrepする。特にコードを削除する修正の時など、削除もれがないように注意する。削除すべきタイミングでしないと、消して良いかわからないコードが残り、これを消すのに勇気が必要になってしまう。（本来自動テストで担保するべきではあるけど）
プルリクの目的に対して、変更点が適切である
機能を追加しようという時に、追加するべきファイルが適切かを見ることで、大枠の設計として問題ないかを確認する。git diffの--name-onlyオプションが便利。ちなみに最近覚えたけど、リモートネームを指定すればローカルにマージ先をチェックアウトする必要はない。例えばmasterにfeature_Aブランチからプルリクが出ている場合は、以下のコマンドで修正ファイルを確認する。
$ git checkout feature_A $ git diff origin/master —name-only
プルリクの目的外の問題を見つけたら、（別の問題であっても）指摘する
これは異論があるかもしれない。本来プルリクの目的以外の修正は、別のプルリクで対応するべきではある。
とはいえ、実務でレビューしていて何か問題を見つけた時、その流れで指摘した方が作業効率がいいのは確か。また、プルリクの目的から本当に大きくそれた問題を見つけることはあまりない（そもそもそういう箇所は見ない）。あえて別の修正を依頼するために担当者にプルリク以外の手段で修正を依頼して、そっちで別のプルリクを作ってもらう手間と、その作業の間にうっかり忘れてしまったりするリスクを考えると、見つけたその場で指摘する方が漏れがなくて良いと思っている。
名前については（クラス、メソッド、変数等）、適切でないものは細かくても指摘する
あえて書かなくても当然だが、コードにおいて名前は本当に重要。もちろんここにはコメントの適切さなども含まれる。可読性が低下するとその後のメンテナンス性が低下するのはもちろん、レビューの効率も悪くなる。
あとスペルミスは絶対に修正する。スペルミスがあるとgrepで見逃したりする可能性がある。
No comments:
Post a Comment