2016年8月4日木曜日

コードレビューで心掛けていること

Twitter の Ads チームは、これまでに働いてきた環境に比べて、コードレビューをずっと重視している。
4 年間を振り返って、自分が学んだことをまとめておきたい。

コードレビューの目的は、ソフトウェアの品質を高めることだ。品質にはいろいろな観点があるが、正しく動く、メンテナンスしやすい、設計が正しい、の 3 点でカバーできると思う。

正しく動く

この観点でのレビューには、ロジカルなバグ、テストカバレッジ、エッジケースを考慮してるかなどの確認が含まれる。
テストカバレッジのチェックやマイクロベンチマークは、理想的には自動化して、変更のマージ前にチェックできると良い・・・が現実的にはいろいろ難しいのでレビューでカバーすることになる。
具体的には、null チェックしてるか、メソッド中の条件分岐がユニットテストでカバーされてるか、無駄にオブジェクトを生成してないか、適切に例外を処理しているか、などを見る。

メンテナンスしやすい

大きなサービスだと、運用を担当するエンジニアが機能開発をするエンジニアと別(またはサブセット)ということがよくあるので、オペレーションの観点からのレビューが重要になる。
この機能がバグってたときに、サービスとしてどのように振る舞うべきかを考え、サービス全体を落とすべきなのかその機能だけを無効にするのか、その機能が動いていないことをどのように誰に通知するのか、などを考える。メンテナンスという観点では、コードの読みやすさも大切なので、長すぎるコードやおかしな変数名・クラス名は直してもらう。
これが良いことかどうかは別の機会にまとめたい(追記:まとめた)が、運用を担当するエンジニアは、リリースする機能やリリースのタイミングについて強い決定権を持つべきで、同時に、ある機能をリリースする / しないことによるリスク・ベネフィットのバランスを持たなくてはいけない。

設計が正しい

多分これが一番重要で難しいが、「この変更にバグがなくメンテナンス性も高いと仮定して、そもそもこれは正しい設計なのか」という観点が重要だ。
このレビューが難しい理由は、正しい設計を思いつくには、現在・将来のサービス設計、この機能のビジネス的な意味、どういう方向に発展していくのか、といった幅広い理解が必要だからだ。レビューワが変更のバックグラウンドを知っているケースもあるし、優れたレビューワだと「この変更を読んだところ、あなたがやりたいことは XXX だと思うのだが、だったらこのようにコードを書いた方が良いのではないか?」と、コードを読む→変更の理由を理解する→正しい設計を提案する、とリバースアセンブルしたりする。

理想的には、コードを書く前にコミッタとレビューワが議論すると良いのだが、すべての機能開発でそれができるわけではない(コミッタにとっては自明な変更でもレビューワにとってはそうではないことはよくある )ので、コードレビューの時点で差し戻されることもある。上述のとおり、コードを書き直すことのインパクトをレビューワは理解するべきで、なんでもかんでも差し戻せば良いというものでもない。ただし、「このように書き直すには X 日かかるが、リリースが X 日遅れるとビジネス上どういうインパクトがあるのか?」と聞くと「じゃあ書き直すわ」となることは経験上よくある。概算で良いので定量化(X 日)することで議論しやすくなると思う。

この観点でレビューするときに、最終形が明確に見えていないときがままある。「最終的にどういうコードになるかはっきりとはわからないけど、この方向で書き直すと良さそうだ」と感じてコメントするときもあるし、そのようなコメントをもらうときもある。結果として良い設計になることもあるし、「XXX という理由でその方向では書き直せなかった」ということもあるが、「多くの場合に、結果的に正しい設計にたどり着く方針を、最終形が明確に見えてない時点で指摘できる」レビューワは本当にすごいと思う。



何度も書いているが、コードレビューする際には、この機能をリリースすることのベネフィットを理解することが最も大切だ。バグがないこと、サービスが安定すること「だけ」を目標にすると「新しい機能は一切リリースしない」のが合理的な解になってしまう。そうすると、重箱の隅をつつくようなコードレビューになり、コミッタとレビューワが対立することになる。レビューワはコミッタと同じ方向を向いて、「この変更のどこを修正すれば、この機能をリリースできるのか」と考えると生産的なレビューになると思う。

0 件のコメント:

コメントを投稿