プログラミングのバグや無駄を減らすために意識してほしいこと4つ

このページに書いてあること

Javaのレビューで見かける4つの見直しどころを、利用者登録とCSV出力の例で確認します。比較のnull、戻り値の候補、定数が表す意味、コメントに残す理由を、それぞれ分けて判断します。

前提

例では、未設定を状態の不一致として扱う比較と、未設定の入力を拒否する処理を別にします。実際のコードでは、入力が欠けたときの動きを先に決めます。

・文字列比較では定数を前に持ってくる

任意の状態を ACTIVE と比べるなら、値が決まっている側から ACTIVE.equals(status) を呼びます。ACTIVE は private static final の文字列定数なので、呼び出し時にnullへ変わることはありません。状態が必須なら、比較をfalseにする前に入力の欠落を検出します。

・処理の結果を判定する為に数字や文字列を使用しない

登録結果を 0 と 91 で返すと、呼び出し側は値の意味と全候補を別に調べる必要があります。既に登録済みであることを通常の結果として扱うなら、REGISTERED と ALREADY_REGISTERED をenumにできます。接続タイムアウトなどの障害とは区別します。

・文字列をなんでもかんでも定数化しない

CSVの列数に制約があるなら、EXPORT_COLUMN_COUNT にすると、数字が何を制約するか伝わります。区切りのカンマを一度しか使わず、String.join の引数と読めるなら、その場に置けます。複数の処理が同じ外部仕様を参照する場合は、変更箇所を揃えるため名前を付ける意味があります。

・意味のないコメントを書かない

コードを読めば「3列をカンマでつなぐ」ことは分かります。ここで残すのは、なぜ列名と列順を変えると連携先の読み取りが壊れるかです。例の4項目をまとめて確認できるコードを載せます。

import java.util.Objects;

public class CodingHabitsExample {
    private static final String ACTIVE = "ACTIVE";
    private static final int EXPORT_COLUMN_COUNT = 3;

    enum RegistrationResult {
        REGISTERED, ALREADY_REGISTERED
    }

    static boolean isActive(String status) {
        return ACTIVE.equals(status);
    }

    static String requireStatus(String status) {
        return Objects.requireNonNull(status, "status");
    }

    static RegistrationResult register(boolean alreadyRegistered) {
        return alreadyRegistered
                ? RegistrationResult.ALREADY_REGISTERED
                : RegistrationResult.REGISTERED;
    }

    static String csvHeader() {
        String[] columns = new String[EXPORT_COLUMN_COUNT];
        columns[0] = "user_id";
        columns[1] = "name";
        columns[2] = "status";
        // 連携先は、CSVの列名と並び順の両方で値を読み取る。
        return String.join(",", columns);
    }

    public static void main(String[] args) {
        check(isActive("ACTIVE"));
        check(!isActive("DISABLED"));
        check(!isActive(null));

        try {
            requireStatus(null);
            throw new AssertionError("missing required status accepted");
        } catch (NullPointerException e) {
            check(e.getMessage().equals("status"));
        }

        check(register(false) == RegistrationResult.REGISTERED);
        check(register(true) == RegistrationResult.ALREADY_REGISTERED);
        check(csvHeader().equals("user_id,name,status"));
        System.out.println("7 checks passed");
    }

    private static void check(boolean condition) {
        if (!condition) {
            throw new AssertionError();
        }
    }
}

javac CodingHabitsExample.java
java CodingHabitsExample
# 7 checks passed