コードレビューで気をつけていること5選

レビュアーとして他人のコードの確認を行うことが多いのですが、その際にどのような観点でレビューを行っているかをまとめてみたいと思います。

これからレビュアーとしての立場を担当するという人や、逆に書いたコードをレビューしてもらう立場の人の参考になれば幸いです。

コード規約に沿っているか？

あらかじめ決めておいたコード規約に沿っているかをチェックします。

例えば次のような規約にしていたとします。

• 変数名：スネークケース
• メソッド名：lowerキャメルケース
• Class名：キャメルケース
• テーブル名：スネークケース（複数形）

メンバーが好き勝手に変数名を定義するとそれが変数名を示すのかメソッド名を示すのかがぱっと見で分からなくなるのでコード規約は厳格にチェックします。

また、一覧データの変数を定義している場合は変数名が複数形になっているかどうかも確認します。

for文のインデックスも$iなどと省略せず極力意味のある変数名になっているかを確認します。

冗長な記述になっていないか？

変数名に関してはどのようなデータを格納するのか明示的にしないといけないので、ある程度冗長な命名でも問題ありません。

ただ、次のような場合はどうでしょうか？

例：管理者ユーザの一覧を表示するメソッド

冗長な例

class AdminUserController extends BaseAdminController
{
   /**
    * 一覧画面
    *
    */
   public function listAdminUser(

シンプルな例

class AdminUserController extends BaseAdminController
{
   /**
    * 一覧画面
    *
    */
   public function list(

AdminUserControllerという名前からAdminUserのデータを扱うことは自明なのでメソッド名はシンプルにするべきです。

車輪の再発明を行っていないか？

車輪の再発明とは「広く受け入れられ確立されている技術や解決法を（知らずに、または意図的に無視して）再び一から作ること 」です。

継承元で定義済みのメソッド、ライブラリやphpの関数で実現できるものを新規メソッドとして再定義していないかチェックします。

少なくとも継承元で定義済みのメソッドがないかをチェックして、存在しない場合に初めて専用のメソッドを作成するようにしてください。

共通化できる部分はないか？

複数のテーブルに対する処理で、各画面で同じ処理が存在していると改修時に何カ所も修正する必要がでてきます。

そういった場合は極力継承元ClassやTraitとして共通化するように指摘します。

view側も同様で、登録ボタン、戻るボタンなど複数画面で共通するものはパーツ化が必要です。

一覧画面のページネーションのパーツなども同様です。

強すぎる言葉で指摘していないか？

レビュー時のコメントにも気をつけます。罵倒しないのは当然として、よほど同じミスを繰り返さない限りは強く指摘することはしないようにしています。

以前大炎上しているプロジェクトにメンバーとして入りましたが、ちょっとしたケアレスミスで人格否定までするような罵倒コメントをもらったことがあり、反面教師として、いくら炎上中で心に余裕がなくても、そういったキツイ物言いはしないように気をつけています。

もちろん普通に丁寧な日本語でコメントすれば問題ないのですが、オフショア開発などで機械翻訳の英語でコメントする場合は、一旦訳された英文を読んで推敲したり、再度日本語に翻訳しなおして意図しない文章になっていないかも確認していました。

品質の高いコードを書くことは大事ですが、なによりメンバー全員が楽しく穏やかに開発を進めることができるのが一番大事ですからね。