この記事はモチベーションクラウドシリーズアドベントカレンダー2021の5日目の記事です。
こんにちは。モチベーションクラウドシリーズのテックリードをしている江上です。
現在弊社では、50名ほどのエンジニアが日々コードを書き、レビューをしています。僕自身もテックリードとして多くのコードレビューを行ってきました。
当然、コードレビューはエンジニアの大事な仕事だとは思うのですが、『思ったより時間がかかる』『何をみたらいいかわからない』というのは誰しもが悩むポイントではないでしょうか？
そこで、この記事では、多くのコードレビューをする上で気づいた僕なりのコードレビューのコツを記していきたいと思います。
効率的・効果的にコードレビューをすることで、コードを書く時間が増えるだけでなく、レビュー観点を知ることでコードの精度向上にも貢献できれば幸いです。
いいコードレビューとは
wikipediaやGoogleで述べられているようにコードレビューの目的として、「コードの一貫性・保守性を向上すること」「バグの隠蔽を減少させること」などが挙げられます。
つまり、「一貫性がないところを指摘できる」「バグを見つけられる」ことができれば、いいコードレビューと言えそうです。ここからは、そのコツを紹介していきます。
コツ1： 変更の意図を把握する
まずは、変更の意図を把握することが大切です。要件定義書や設計書を読んだり、コードを書いた人にヒアリングを行うことで、このコード変更で得たい成果を知りにいきましょう。併記されているテストコードも参考になります。
また、コードの有効期限を知るのも大切です。緊急の暫定的な処置なのか？将来を見越した変更なのか？で求められるレビューの観点も変わってきます。
コツ2: 順番は大->小、内->外
変更意図が把握できたら、次はコードを読んでいきます。順番は大->小、内->外です。
大->小とは、全体をざっと見た後で細かいところをみるということです。全体を一度通してみることで、想定していなかった変更差分が見つかったり、名前の一貫性をみることができます。
内->外とは、データの流れを見た時の内（データベース）から外（view）の順番でみることです。Railsだとmigration -> model -> controller -> viewの順番で見ます。データベースの変更は特に注意が必要です。ORマッパーを使っている場合、名前はそのままメソッド名になりますし、nullの制約やindexが適切に貼られていないと意図しないデータが入ってしまい必要以上に複雑なアプリケーションになってしまいます。自分自身、時間がない時はmigrationと削除コードだけみることも多いです
コツ3: 削除コードは特に注意してみる
経験上、バグが起きるのは追加コードよりも削除コードの方が多いように思います。追加コードは自分が作った機能でしか使われてないですし、作った機能はしっかりテストされてるので問題は起こりにくいです。一方で削除コードは、今までどこかで使われていたはずのものです。他のメソッドで実は参照していてバグを引き起こしたり、削除したメソッドの中で読んでいるメソッドも消さないとゴミが残ったりします。
コツ4: 追加コードは名前に注意
追加コードで特に注意したいのは一貫性・保守性です。前述の通り、ちゃんとテストする時間やフローが整っていれば追加コードで機能的なバグが出ることは少ないからです。追加されたメソッドや変数の名前に一貫性があるのか？や、既存のメソッドで代替できないか？などはチェックしておきましょう
コツ5: 処理を想像する
追加コードでバグがあるとしたら、非機能面や異常系の処理ではないでしょうか。性能やセキュリティの観点で問題がないか確認しましょう。その際問われるのは想像力です。発行されるクエリを想像し、「数が多くなりすぎないか？」「joinのしすぎでメモリが落ちないか？」「レコードはどういうスピードで増加するか？」考えたり、「パラメータを改竄したらどうなるか？」などさまざまな観点で想像力を働かせましょう。想像力を働かせるヒントとして、コードレビューの観点をおまけに記載しています。
おまけ: コードレビューの観点
以前投稿したレビューする人も、される人も知っておきたい！開発プロセスごとのレビュー観点チェックリストの中から、コードレビューの観点を転載しておきます。
Rails特化な内容ですが、観点は同じだと思うので言語やフレームワークに応じて読み替えていただければ幸いです。
他にも、Googleさんがコードレビューの観点を公開してくれていたりするので、ぜひ参考にしてください
プルリク
- PRの粒度が適切か？（機能ごとにPRが分けられているか？）
- コメントで変更点だけではなく、変更理由も記載されているか？
名前
- わかりやすい名前になってるか？
- マジックナンバーがないか？（例: 特別な意味を持つ数字や文字列は定数で定義しているか）
- 同じものには同じ名前が使われているか？
一貫性・保守性
- 消えているべきメソッドが消えているか？
- コードや名前に一貫性があるか？
- 同じ処理をしているメソッドが既に存在してないか？
データの検証
- 変数がnilだったとき落ちるようなコードになってないか？
- selectやupdateするときに対象としているstatusは正しいか？
- 最新のレコードを取得する時に参照するカラムが正しいか？
データベース処理
- 適切なトランザクションが張られているか？
- ロックが適切か？（ちゃんと開放するか？不用意な範囲ロックを取得してないか？）
システム効率
- n + 1が発生していないか？
- 容量やレコードが多いテーブルとjoinしてないか？
- ループ内での初期化や無駄なクエリ発行がないか？
- Array#find or Array#select etc..で配列内をループしても、問題ないデータ母数か？
- map(&:id) => pluck(:id)に書き換えられないか？
コメント
- メソッドの内容ではなく、意図を記載できてるか？
ミスしにくい書き方
- importメソッドにて、理由がない限り、on duplicate key updateはfalseで指定されてるか？
- transaction内で非同期処理の呼び出し（perform_later)などをしていないか？
- reloadではなくfindが使われてるか？（reloadは原則禁止！）
テスト
- CIが正常終了しているか？
セキュリティ
- action単位で認可処理が問題ないか？
- パラメータを改竄しても問題ないか？
エラー
- エラーを握りつぶしてないか(正当な理由がない限り)