初めに
弊社では以前はレビュアーが何人かいて、そのうちの1人を指定してレビュー依頼を出すというフローでコードレビューを行なっていました。
ある日、
「レビュアーは指定しないでコードレビューを出しましょう！」
「誰でもレビューしていいようにします！」
というふうになりました。
主な理由としては
- レビュアーを指定しないようにすることで1人に依頼が偏らないようにする
- 誰でもレビューすることで技術力を上げる
といったことがあります。
技術力を上げていきたい人は積極的にレビューしていきましょう！
と言われても、何をみて、何を言えばいいのかわからなかったので調べたこと、思ったことをまとめます。
ここに書いてある事は、レビューしてもらう側も意識することでコードがレベルアップすると思います。
ダメ出しじゃない
まず、
- コードレビュー＝ダメ出し
みたいに思ってました。
自分は人の書いたコードにあーだこーだ言えるほど偉くないし、知識も自信もないなと思っていたのですが、
色々調べてみて思ったのは、
- コードレビュー ＝ コードに対する意見交換機会
なのかもしれない。ダメ出しではないのかもしれない。
そう思うとレビューするハードルが下がる気がしました。
レビューのレベル
レビュアーのレベル
- ①diffで見えている部分の問題
- ②diffで見えていない部分の問題
が見えるかどうか
①diffで見えている部分の問題
①は比較的簡単。見えているから。
例えば、コーディング規約違反やタイポ、ifの条件が間違っていること、名前が分かりにくい、意図がわからない書き方がある。など
②diffで見えていない部分の問題
②は結構難しい。
変更を加えていない部分への副作用や、将来問題となる種がないかなどをチェックする。エンジニアとしての経験がものをいう部分だけれど、自分で実装するならどこに気をつけるかを意識して変更点をみる、悪いコードの典型例を頭に入れておくなどをすると1段上のレビューをする足がかりとなる様です。
コードの不吉な臭い
初心者がレビューするときのポイント
- わからない、わかりにくい部分を質問する、指摘する
- わからない、分かりにくいと感じたということはコードに改善点があるのかもしれないということ。知らないメソッドだったら最低限調べる。
- 複雑な条件などはテストコードを書いてもらう
- 細かいスタイルの修正もちゃんとお願いする
わからない部分を質問して、説明してもらうことで
- 書いた人、質問した人の理解が深まる
- 深まった理解で間違いに気がつくかもしれない
- わかりにくいなら書き直す、コメントを残すなど更なる改善がされるかもしれない。
と質問することはいいことがたくさんあるので臆せずに質問する。
他にも見るポイント
- 境界値
- テストコードが正しく失敗するように書かれているか
- 分かりにくい名前
- 他と違う書き方、珍しい書き方をしている部分
- なぜそう書いてあるのか残した方がいい。または書き直す
- 1つのメソッドにifが複数ネストしている
- メソッドを設計し直した方がいい
- ユーザーの入力値を受け取る部分
- XSS/SQLインジェクションなど
- 3分でわかるXSSとCSRFの違い
- SQLインジェクションとは？仕組み・被害事例・対策をわかりやすく解説
指摘が間違っていたら...
上のポイントに従ってコードをレビューして、もし指摘が間違っていたらどうしようと尻込みしてしまいますが、
そんなときはきっと
「ここは〇〇だからこうやって書いたんだよ。だから問題ない。」
など返してくれると思いますし、書いた理由などがわかるので自分の知識も増えると思います。
またそうやって書いた理由を説明してもらって重要だと思ったらコメントに残してもらうのもいいかもしれません。読んでわからなかったのなら他に読む人も同じ疑問を持つポイントかもしれないです。
終わりに
自分は誰でもコードレビューをしていいという環境にいるので積極的にレビュー依頼を取っていきたいと思いました。自分がレビューするときは
コードレビューはダメ出しじゃない
と思ってやっていこうと思います。
チェックリスト
- 不要なコメント、デバッグコードが残っていないか
- わかりにくい部分は質問する
- コーディング規約違反やタイポはないか
- if文の条件がおかしくないか
- 命名がおかしくないか
- テストコードを書いた方がいいか
- 境界値の扱いは適切か
- 他の箇所と書き方がそろっていない部分はないか
- ユーザーの入力をエスケープするなど、正しく処理できているか
- コードの不吉な匂いに該当しないか