コードレビューの質を上げるために事前に確認しておきたい5つのこと

もはや当たり前のコードレビュー

2人以上で開発をしていて、コードの質を上げたり、お互いに成長するために、まず行われているのがコードレビューだと思います。 他の人にコードを見てもらうことで、どこが悪いのかを指摘してもらったり、今まで知らなかった書き方を教えてもらったりします。

教育の観点でも優れていて、学べることも多いので、最初は書いたコードを見てもらう機会が多くあるはずです。 新米プログラマだから多少コードがひどくても許してもらえるかもしれませんが、 コードレビューで突っ込まれる可能性が高い部分はなるべく減らしておいたほうが、コードを見る人にも見てもらう人にも負担が少なくてすみます。 円滑なコードレビューを行うために、新米プログラマがレビューの申請を出す前に注意しておくといい項目をまとめてみました。

あまり、特定の言語に依存しない内容にしてあります。

もし、記事を読んでいて、「こんなの当たり前じゃないか、もう少し高度な内容をしりたい」という人がいらっしゃいましたら、 手前味噌ですがこちらのスライドも参考にしてもらうといいかもしれません。 ソニックガーデンで取り組んでいるコードレビューについても書かれています。 デキるプログラマだけが知っているコードレビュー7つの秘訣

1. コピーしてきたから正解ではない

よくあるパターンに、同じプロジェクトで似たような実装があったので、そこを参考にしました と説明することがあります。 しかし、同じプロジェクトで実装されていたからといって、それが本当に正しいコードだという保証はありません。 一度コードレビューを通っているはずなので、コードの質は担保されているかもしれませんが、 もしかしたら、記述が古いかもしれない（Railsだとよくある）ですし、昔書かれたコードでリファクタしたい場所なのかもしれません。

2. 将来必要なものは今は必要でない

今度○○を実装するときに必要だと思って、あえて残してあります。 みたいなこともよくあります。 しかし、それだと 本当に将来使われるかわからない、現時点で無駄なコードが本番環境に反映されてしまいます。 これは YAGNI(You ain’t gonna need it)の原則 と呼ばれるくらいプログラミングではよく遭遇する場面です。 プログラミングのスキルが向上すると、保守性や可読性だけでなく、拡張も容易なコードになっていきます。 なので、将来を見据えてコードを書くのでなく、その時点で必要なものだけを実装することを心がけると、自然とコードの量も減り、 意図が伝わりやすい実装になります。

3. コードで表せなくなって、はじめてコメントを利用する

何をしているコードなのかが伝わるか不安になり、コメントを必要以上に書いてしまうと、逆に「なんでこのコメント書いたの？」 と質問されてしまう場合があります。大抵レビューしてくれる人は自分よりプログラミングが出来る人のはずなので、 メソッドや変数の命名が適切であれば読み取ってもらえます（その命名が難しいんですが…）。

では、どういう時にコメントが必要かというと例えば次のような場合です

# 会社設立が2009年なのでそれ以降を選択肢に表示
= f.input :hired_date, label: '入社日', start_year: 2009, end_year: Date.current.year

上記のコードは入社日を入力してもらうフォームの例ですが、 もし突然start\_year: 2009というコードが現れると「なんで2009が決め打ちで出てくるんだ？」 と疑問が出てきます。このように コードでは表現できない部分にコメントを書いておくと、コードを見ている人を悩ませなくてすみます。 （この例の場合だと、コメントでなくてもestablished_year = 2009みたいな変数、定数でコメントをなくすこともできますが）

4. テストとマージは大丈夫？

これはコミット毎にも確認しておきたいことですが、コードレビューをしてもらう時には、 テストが通っていて、マージ可能か を確認したほうがいいです。 もし、テストが通っていなければ、まだ作業が完了していないことになりますし、マージできない状態だったら、 「コード的にはいいけど、マージできないので修正してね」といった無駄なコメントのやりとりが増えてしまいます。 OKだったらそのまま取り込んで貰えるコード を出したいですね。

5.　新しく生まれた問題は別issue

コードレビューを一度してから指摘されたところを修正しているときに「ここも直す必要あるのでは？」や 「ここコメントで注意されてたから、一緒に直してしまおう」と、 最初のコードレビューの内容とは異なる部分を新たに実装してしまうと不都合な場合があります。 もし、加えた実装の部分に、また指摘が入ってしまったら、最初の目的は実装されているはずなのに、 マージすることができない状況なってしまいます。 すると、コードレビューに関わっていない周りの人からは仕事の進捗が進んでいないように見られてしまいますし、 レビューをする人も既に見て大丈夫だった部分を、もう一度見なければならなくなります。

もし、新しく注意された場所や、他にも直したい場所があるなら、別のチケット（issue）を作り、 まずは、今取り組んでいる部分をマージしてもらうと、仕事の進捗も進んでいることが把握できますし、 コードレビューの人の負担も減るはず です。

本当にベストなコードか？

以上5つのことを書きましたが、私の場合コードレビューの前には、師匠からこの言葉を毎回言われます。 常にベストなコードまでいかないにしても、ベターなコードを目指して無駄なやりとりを発生させずに、 自分の学びになることを効率的に学習していきたいですね。