コードレビューのチェックポイント

業務でレビューを行うようになってから2年ぐらい経つ。(自分が入社する前を含めてそれまでのずっと長い間、レビューが全く行われなかったというのも恐ろしい話だ。)

いきなりレビューアになっても何を見ればいいのか、何に気をつければいいのかわからなかったが、だんだんとわかってきたような気がするので、ポイントをまとめてみる。
(日頃の業務ではAndroidのJavaをだいたいレビューしたり、してもらったりしている)

Sponsored links

前提 (何のためのレビューか)

コードレビューを何のためにするのか、最初に認識合わせをしておけると良いと思った。
自分たちの場合は、「コードの共有」が目的だった。
どんな機能が入ってどう実装してあるのかを開発者同士で共有することが目的だ。
他の人の修正で自分的にLGTM(Looks Good To Meの略?)なら指摘が全然無しでもOKという理解だ。
不具合とか、より良い構成とか、変な書き方とかを指摘できればなお良いけど、お互いが自分の担当分の開発もありながら、他の人の対応をレビューしていくという並行作業の中、いかに効率よくできるかも大切になってくる。そこで何を目的としてそこは最低限抑えておければOKというレビューの目的や前提を抑えておきたい。

チェックポイント

自分の環境でビルドできるか

特に静的型づけ言語の場合、コンパイルできないと話にならない。
そんなビルドが通らないコードをコミットする訳がないと思ったら、何かの間違いでタイポしてエラーになっていることがあった。gradleでライブラリを追加した場合など、キャッシュが影響していたりして、自分の環境だと問題無いけど、他の環境だとsyncできなかったなんてこともある。

メソッドの呼び元の確認

そのメソッドがどこから呼ばれているか、呼び出し階層を調べてみる。
Androidの場合、Application#onCreate で 初期化を入れた際に マルチプロセスで起動されるおそれがあるので、プロセス限定で起動させたい処理は気をつける。
※ Preferenceの複数プロセスからのアクセスで壊れるので特に注意したい。実際に壊れた。

その処理キャッシュできないかどうか

メソッド内で負荷のかかるDB接続処理などに何度もアクセスしてたりする場合、一回変数に取得して使いまわしたりできないか。

リソースのとじ忘れは無いか

close忘れないか。try resourceで簡単に書ける場合など

クラス名やメソッド名の誤字脱字など

クラス間で微妙に違うとか、英語のスペルが間違っているとか、自分では気づきにくい

既存の処理と似かよった機能の実装時のコピペからの変更忘れ

コピペした後に変えないといけない部分など

定番機能の実装忘れ(タイプ毎のケース分けなど)

定番処理でも複数処理を分けるケースなど、特定のケースが抜けていないか

インスタンス化の際に代入し忘れているフィールドが無いか

マッパーとかのメソッドで、引数のオブジェクトのフィールドをマップ先のオブジェクトのフィールドへの代入を忘れがち。リファクタ中に追加した変数は忘れられたのかなと思いをはせる。

実際に動かして見た目でエラーだとわかる部分は注力しない。わからない部分をチェック

非同期処理など、リリースされて大勢のユーザーが使った時などに確率的にエラーとなる部分などに気づけるとヒーローになれる。
表面的に動かして明らかにエラーだとわかる部分はそのうち誰か気づくだろう。。

使っていないimportをチェック

Lintとかで機械的にわかるのかもしれないけど、そんなもの導入していないので。
リファクタリングしているとよくやりがち

簡単にかける条件判定

メソッドでboolean 返すものに hasParents() == true とかしている場合がある。簡潔に == true いらない

境界値のチェック

10個なのに11個取得していないかなど

ライブラリでできることを再開発していないか。

ライブラリ使って一行で書けるところを独自にメソッド使って処理していたり

ネストを浅くできそう

早めにreturnして複雑にならないようにできないか

同じ処理をまとめられそう

コピペして同じことを複数の場所に書いていたらどこかのクラスなりメソッドにしてくくり出せないか

まとめと気づき

以上、他にもあったら随時記載して、まとめ直したい。

自分より経験がある人のコードをレビューするのはかなり敷居が高いと感じる。
自分なんかがレビューできるのかといつも感じている。しかしそこはチャンスだと前向きに捉えてすすんで行いたい。

インプットのチャンス

  • レビューアの場合
    他の人のコードからインプットできるチャンス。特に自分より経験がある人のコードを見て、こんな書き方があるのか、こんな構成、アーキテクチャで設計するのかといつも勉強になる。わからないことは素直に聞いてみると、教えてくれると思う。

  • レビューイの場合
    アウトプットしたことに対して、他の人からフィードバックをいただけるというのは成長できるチャンスだ。ここを見ていただきたいとか、もっといい実装方法があれば教えてほしいなど、レビューのコメントで注力してレビューしてもらいたいポイントを記載するのもいいと思う。

コミュニケーションのチャンス

開発者同士コミュニケーションが普段から取れていれば、レビューもしやすくなると思うが、なかなかいい感じの関係って築けていないこともあると思う。
コードレビューを通してその人について感じられることもあると思う。
どういうことを大切にしている人なのか、何に注力したいのかが見られてお互いのことを知ることができるチャンスだと捉えられる。
また同じ人のをレビューしていると、毎回よくやりがちなパターンというのがだんだんわかってくるような気がする。
代入するフィールドを忘れがちとか、フィールドを後から追加した時に代入忘れたのかもとか思いをはせる。

わからないことは聞いてみる

これはかなり大事だと思う。
もしかしたらその人も説明することで何かに気づけるかもしれない。
「〜というのは、〜という認識で合っているでしょうか?」
「同じIDでも複数登録される場合があり、その場合は正しい状態ということで合っているでしょうか。(xとyが同じ場合は同じID)」
などなど、聞き方を工夫して教えていただく。

Comments

タイトルとURLをコピーしました