概要
いきなりの自己紹介になりますが、私はプログラムを本格的に書き始めてちょうど1年半ぐらい経とうとしています。コードを書く機会がメインでしたが、最近になって見る側になることも多くなりました。
コードレビューというものをする立場として、「何を見ればいいのか」とわからないこともあったので、知識ある方々のレビューを観察し、見方の方針を観察していました。
私がレビュアーとして立ち振る舞うときは、上記で学んだこと、つまり本記事の内容を強く意識するようになりました。そうすれば自ずと指摘する立ち回りもうまくなってきたという感じですね。
私は現在Java技術者であり、内容はJavaを中心に記載しております。ですが、極力他言語も考慮し一般化した内容で書きたいと思います。
コードレビューのやり方
私が観察していると、大体指摘されるのは以下3点かなという印象です。
- 命名規則
- バグの起きうる箇所の確認やキーワード
- クラス構成などアーキテクチャの観点
もちろん他もありますが、大体この3つな気がします。細かく見ていきたいと思います。
命名規則
できる人のコードレビューこそ、ここが一番厳しい。
コードレビューとは「コードを読むこと」がすべて。読めないソースコードとは、
- 命名が意味不明で文脈が破綻している
- 抽象的すぎて文意が伝わらない
- 読者の実力不足
の大体3パターンかなと最近感じています。
読者の実力不足
最初に言うと3つ目はどうしようもない。勉強しましょう。以上です。
これは私も日々痛感しています。ただ勉強のみです。
命名が意味不明で文脈が破綻している
では意味不明な文脈とは何でしょうか。例が雑ですが次の場合が考えられます。
public String buildSql(int a) {
StringBuilder count = new StringBuilder();
count.append("SELECT")
// 以下略
}
極端ですが、こんなイメージです。countという変数、aという変数では各変数たちの役割がわかりません。
変数名、メソッド名、クラス名、パッケージ名、これらは具体的に正しく付与すべきです。
（もちろん抽象クラスやインタフェースなどのように抽象的な名前を与える場合もあります）
例えばClean Architectureなどのモデルでは、adapter/usecase/infrastractureと処理が明確に区別されています。adapterあるいはcontrollerは外部からの処理を受けるゲートウェイとして働く。
ここでBatchTaskControllerとでも名をつけてあげれば、REST APIなどを経由してバッチタスクの処理要求を受信するのかな？という大雑把な処理が見えてきます。
プログラムはストーリが成り立っており、それが正しくすらすらと読めるようにすべき、これはコードを書き上げる上での「前提」であると感じています。
最悪一本道な汚いクラス設計でも、一本道で読めるようなきれいなロジックと「名前」があればいいのです。逆にクラス設計のベストプラクティスに沿っているものの、命名が無茶苦茶なら読む気の失せる文章になってしまいます。
（一本道の悪いコードは以下3つ目のポイントで確実にはじかれますが...）
作者の気持ちを考えて文章を読む。中高校生の学ぶ受験のための国語は案外ここで生きてくると思っています。
抽象的すぎて文意が伝わらない
例えば
public int check(int a) {
a = a++;
if (a < 100) return a;
return 0;
}
という処理があったときに、checkって何をチェックするんだ？intを返しているがこのintは何か？という「疑問」が出た時点でアウトです。
具体的に何をやっているのか、返す値は適切で何を返しているかわかるかがポイントです。
例えばvalidateBatchTaskParameter()のような、「何の何を、どうするか」という命名をしてあげることで、呼び出し元が読みやすくなります。
特にアメリカのような、英語を第一言語とする国のプログラマは多少命名が長く、その分丁寧な印象を感じました。
以上を踏まえると、プログラムとは文章であり、「読めるもの」を提供すべきというのがコードレビューの最優先確認項目になります。
もちろん実力不足による「読めない」もあります。素直に質問するのも大切なポイントだと思っています。
キーワードとバグをつかむ
キーワードを見つける
キーワードとは、コードを読む中で出てくる「？」となる箇所です。
なぜそこをThread化しているのか、なぜそこでチェック処理をはさむのか、なぜそこで○○をするのか。作成者には意図があるはずです。
その意図は、プログラムとして本当に正しいかを見極める必要があります。
チェック処理やリソースの獲得と解放などは実装方法を間違えるとバグにつながります。そこを重点的に見ていきます。
私もこれは最近意識するようになりました。ですがかなり難しいです。経験が物をいう箇所だと思っています。
Javaの場合、前述の通りThread使用箇所やsynchronizedしている箇所、チェック処理などがキーワードになります。他にもNullPointerExceptionが起きうるコーディングやSQLのプレースホルダー使用有無、サニタイズ処理など、バグや脆弱性があるかという観点ですね。
アーキテクチャを把握する
クラス設計は妥当か、これはストーリを把握した後に、全体像の流れが正しいかを確認します。
例えばWeb3層モデルであるか、MVCを意識した設計か。
アーキテクチャの誤りは動作ではなく保守の観点で致命的な傷を与えます。組織が指定したアーキテクチャであるか、一般論として正しいアーキテクチャか、両面からアプローチして確認すべきです。
例えばCLIで動作するシステムにおけるViewクラスは、System.out.println()のような表示処理のみであるべきで、ViewクラスがDBへのアクセスという責務を持つべきではありません。
他にもServiceクラスがユースケースを実現すべきであり、Controllerクラスが直接的にrepository、つまりDBアクセスしてはならないという慣習（プラクティス）があったりします。
要はクラスにいろいろ詰め込んでないか、責務は正しいかという着眼です。
飲食店の業務で、ホールスタッフとキッチンスタッフは各々の仕事（料理の提供/調理）に専念すべきであり、ホールスタッフが調理しつつ提供も行うとなれば、誰が何をすべきかわからず、職場は混乱するでしょう。それと同じです。
総括
同じことの繰り返しで、冗長な文章になってしまいましたが、読めるプログラムを書こう。これがすべてな気がします。
変数やメソッド名は子供のように、意味を考えて命名し、簡潔で明快なプログラムにするのが大切だと思っています。
もちろん複雑になる処理もあります。そこはコメントという素敵な機能で補えばいいのです。
以上、私も精進したいと思います。