🔰

初心者に返すことの多いレビューフィードバック

2022/06/06に公開

はじめに

基礎研修が終わり、2022 年組もコードを書く機会が増えている頃ではないでしょうか。インターンなどでプロダクションコードに慣れている一部の人を除いて、本格的なコードレビューを受けた機会は少なく、フィードバックの多さに困惑している人も多いでしょう。

このようなプロダクション開発の初心者に向けて、私が行った直近 5 年分のコードレビューから、『仕様・プロジェクトルールに依らない』フィードバックの傾向を紹介します。今回調査をしたのは、私がフィードバックした範囲ですが、周囲のレビュアーも同様の印象を持っていましたので、一般的に近い傾向にあるのではないかと考えています。機会があれば、全社の傾向を調べてみたいですが、集計が大変なので...（いずれ AI で分析したいなと画策中）

5年分のコードレビューの統計

今回は、2017 年 6 月から 2021 年 5 月までの 5 年に、私にアサインされたプルリクエストのフィードバックを、集計しています。なお、この期間は、比較的コードレビューが多かった期間です。

項目	数値
レビュー件数(A)	2,820件
合計フィードバック件数(B)	10,659件
平均フィードバック件数(B/A)	3.78件/レビュー

これらの中で『仕様・プロジェクトルール』に関するものが約 63％あったため、それらを除いた 3,944 件を、今回の対象としています。

これらのフィードバックを、『構造に関する指摘』『ネーミングに関する指摘』『例外処理に関する指摘』『ログ出力に関する指摘』『その他の指摘』の 5 つにカテゴライズしました（複数の指摘が混ざっている場合は、指摘の理由として一番大きかったものでカウント）。

カテゴリ	指摘件数
構造に関する指摘	1,957件
ネーミングに関する指摘	789件
例外処理に関する指摘	15件
ログ出力に関する指摘	591件
その他の指摘	592件
合計	3,944件

『構造に関する指摘』『ネーミングに関する指摘』の 2 つで、全体の約 70％を占めています。直感的には、『例外処理に関する指摘』『ログ出力に関する指摘』をもう少ししていた感覚がありましたが、印象に残っていただけで、そこまで多くはありませんでした。

カテゴリ別の代表的なフィードバックの例

ここからは、カテゴリ別に代表的なフィードバックを紹介します。

構造に関する指摘

1. アルゴリズム・データ構造の選択が良くない

たとえば、次のような指摘です。

選択したデータ構造が適切ではない。
Set が望ましい箇所で List を使っているとか。
選択したアルゴリズムが適切ではない。
Unstable なソートアルゴリズムを利用しているとか。
新発想の何かを生み出して論理的に破綻している。

問題に対して、望ましいアルゴリズムやデータ構造を選択することは、性能やリソース問題を避けるだけでなく、維持保守する上でも大切な要素になります。なぜなら、使用するアルゴリズムやデータ構造が、処理の特徴を明確に表すからです。アルゴリズムやデータ構造を正しく使い分けできていない人の多くは、要件や仕様も正しく理解できていないことが大半でした。

基本アルゴリズムやデータ構造は、フレームワークやライブラリで提供されていますので、慣れているものだけを使うのではなく、用途に合わせて使い分けることを心がけましょう。

2. メソッドやクラスの責務が適切ではない

クラスやメソッドが多重責務になっているのは、非常に多いフィードバックです。開発者の中には、クラスやメソッドを増やすことに抵抗を感じる人もいるようですが、用途が混ざることにメリットはありません。

たとえば、『株式の注文を受け付ける』というユースケースを単純化すると、次のようなステップになります。

場中か受付時間内外かを判定する
口座の取引制限をチェックする
余力審査する
銘柄のステータスをチェックする
TSE に発注する
注文を登録する

これらは、各ステップがサブユースケースとして独立するため、クラスやメソッドは分かれます。これを 1 つのクラス・メソッドで実装すると、可読性や保守性の低いものになります。

件数は少ないですが、これとは逆で、クラスやメソッドが細か過ぎて責務が不明確というフィードバックもあります。上の例でいうと、『5. TSE に発注する』を、『現物注文を TSE に発注する』『信用注文を TSE に発注する』『プライム市場銘柄の注文を TSE に発注する』などのように、分けてしまっているケースです。 TSE への発注の観点で見ると、これらを業務や IF 仕様から考えても電文パラメータの違いでしかなく、クラスやメソッドを分ける意味はありません。また現引や現渡のように、現物と信用をまたがる注文の存在を考えると、クラスやメソッドを分割し過ぎることが用途を不明確にする原因となります。

3. 機能選択の引数がある

今回見たものとして、『エラーを握り潰すフラグ』『ソートキーを切り替える引数』『商品ごとの処理の違いを切り替える引数』のようなものです。機能を切り替える引数は、引数の存在と意味を知る必要があるため、分かり難いメソッドになります。ユースケース実装の場合は、用途を明確に表現されていることが重要ですので、クラスやメソッド定義をサボることは止めて、分かりやすく実装しましょう。

たとえば、『商品を登録する』を実現する場合の、次のような実装です。

public class Product {
    /** 商品種別ごとの登録処理をします */
    public Hoge register(Hoge data, ProductType type) {
        switch type {
            case: A:
                break;
            case: B:
                break;
        }
        return data;
    }
}

登録処理の呼出元で、商品ごとに呼ぶメソッドが同じになるから、これで良いとの意見も聞きますが、果たしてそうでしょうか。登録処理だけ見ればそうかもしれませんが、一般的に商品が異なれば、登録以外の処理も商品ごとに異なっていることが普通です。つまり商品の違いによる分岐は、このメソッドの外で、既に判断を必要としていることが容易に想像されます。このような場合は、ユースケースの先頭で商品別にクラスを分ける（Strategy パターン）か、またはその違いが小さいのであれば、メソッドを分ける実装が保守性の高いコードになります。

商品別クラスの例

/** A商品 */
public class ProductA extends Product<ProductA> {
    /** 登録処理をします */
    @Overwride
    public ProductA register(Hoge data) {
        data = super.register(); // 商品共通の登録処理
        return this;
    }
}

/** B商品 */
public class ProductB extends Product<ProductB> {
    /** 登録処理をします */
    @Overwride
    public ProductB register(Hoge data) {
        data = super.register(); // 商品共通の登録処理
        return this;
    }
}

メソッドを分ける例

public class Product {
    /** A商品の登録処理をします */
    public Product registerA(Hoge data) {
        data = register(Hoge data); // 商品共通の登録処理
        return this;
    }
    /** B商品の登録処理をします */
    public Product registerB(Hoge data) {
        data = register(Hoge data); // 商品共通の登録処理
        return this;
    }
    private Hoge register(Hoge data){
        return data;
    }
}

4. 価値のない引数の書き換えはやらない

ユースケース実装における引数の書き換えは、ほとんどの場合『ドメイン欠乏症』を引き起こしています。つまり、オブジェクトの更新方法は、オブジェクト自身が知っており、必要な情報を外部から与えられて更新できていません。

また『コマンド・クエリの原則』から見ると、引数を書き換える処理を提供するメソッドは、コマンド・クエリのどちらでもない存在であり、それを提供するクラスもまた中途半端な位置付けになってしまいます。

なお、業務色のない汎用ライブラリでは、引数を書き換える方が良い場合もあります。

ネーミングに関する指摘

1. システムハンガリアン記法は不要

システムハンガリアン記法とは、String strName; List<Customer> custList; Map<String, BigDecimal> priceMap; のように、変数名に型を含める命名ルールです。 IDE が発達した現在において、変数名で型を表現するメリットはありません。それどころか、型を変更する場合に変数名を変える必要があるなどのデメリットの方が大きくなります。

変数名には型の情報を含めず、String accountName; List<Customer> customers; Map<String, BigDecimal> priceByProduct; のように、明確で分かり易い名前を付けましょう。

2. 実態に合わない名称

クラス名、メソッド名、変数名など全てに共通して言えることですが、『名は体を表す』ことが何よりも大切です。もちろん、テーブル名やカラム名に依存している場合など、変更が難しいものもありますが、実態に合わない名前は混乱の原因でありデメリットしかありません。

ログ出力に関する指摘

ログが多過ぎる、少な過ぎる、出力メッセージが不適切などです。ログの多い少ないは、対象システムの特性によリますので一定のパターンはありませんが、出力メッセージについては以下のフィードバックが大半です。

機微情報を含めてはならない。
状況を特定できる情報が含まれていない。
メッセージが意味不明、または実態に合っていない。
ERROR や WARN のメッセージは、アサーティブに書いて欲しい。

例外処理に関する指摘

例外処理の多くは静的解析ツールで検知されるため、レビューにまで回ってくることは稀ですが、『なんでもキャッチするな』『なんでもレイズするな』は、今でも時々発生するフィードバックです。

その他の指摘

整理しきれないほどありますので、目立った物を羅列しておきます。

NPE になるよ。
否定の分岐、二重否定の分岐は止めて。
OOME になるよ。
トランザクション境界が違うよ。
分岐・ループのネストが深過ぎる。
コメントには意図や目的を書いて。
端数処理が正しくない、または有効桁数が不足している
Race Condition になるよ。
デッドコードは消して。
無駄なキャストが多い／暗黙キャストに頼り過ぎ。
車輪の再発明はやめてフレームワーク・ライブラリを使おう。
フレームワーク・ライブラリの使い方を間違えているよ。

まとめ

いろいろと書きましたが、振り返ってみると、毎年同じような指摘を繰り返しているなと再確認しました。基本的には、『構造に関する指摘』『ネーミングに関する指摘』の 2 つを気を付けるだけで、レビューフィードバックの大半を減らすことができます。

良いとされることは、時代と共に変化して行きます。レビュアも、新しい考えを取り込み、良いフィードバックを心掛けたいものです。