現場でありがちな『全部String問題』をDDD風にリファクタリングしたら心が救われた

前書き

• 当然ではありますがコードは公開しても問題ないよう本記事用に作成しております。
• ただし、似たようなコードが現場に存在していたことは事実です...。

本記事で主張したい事

• 値をやコレクションを扱うクラスを作成し、クラスの責務を分けることでコードの可読性や変更容易性があがったよって話。

今回問題とするコード(サンプルとして渡されたコード)

TL「カルダモン君、サンプルコードはできてるからこれ参考にしてね」
ﾜｲ「おっ、ありがとうございます...！？！！！！？！！？」

/**
 * 学生の名前と科目ごとの点数を管理するサービスクラス
 */
public class StudentGradeService {
    /**
     * 学生情報と科目ごとの点数を登録する
     * @param studentId 学生ID（10文字）
     * @param remarks 備考（0～100文字）
     * @param studentsScores 科目ごとの点数（0～100点、1科目以上必須）
     * @throws IllegalArgumentException 入力値が不正な場合
     */
    public void register(String studentId, String remarks, Map<String, Integer> studentsScores) {
        // 科目ごとの点数が未設定または空の場合は例外
        if (studentsScores == null || studentsScores.isEmpty()) {
            throw new IllegalArgumentException("Scores cannot be null or empty");
        }
        // 学生IDが10文字でない場合は例外
        if (studentId == null || studentId.length() != 10) {
            throw new IllegalArgumentException("Student ID must be exactly 10 characters long");
        }
        // 備考が未入力またはnullの場合は例外
        if (remarks == null) {
            throw new IllegalArgumentException("Remarks cannot be null");
        }
        // 備考が100文字を超える場合は例外
        if (remarks.length() > 100) {
            throw new IllegalArgumentException("Remarks must not exceed 100 characters");
        }
        // いずれかの点数が0未満または100を超える場合は例外
        if (studentsScores.values().stream().anyMatch(score -> score < 0 || score > 100)) {
            throw new IllegalArgumentException("Scores must be between 0 and 100");
        }

        // ここでは登録処理を行う
    }
}
    

なにが問題なの？

責務を持ちすぎている

「バリデーションチェックどこに書こう…。
StudentGradeServiceクラスのregisterメソッドに書いちゃえ！」
…本当にそれでいいのでしょうか？
サンプルコードでは「studentId」「studentName」「studentsScore」3つだけのバリデーションで済みますが、
実務ではもっと多くのパラメータのバリデーションチェックが必要でしょう。
registerメソッドがバリデーションチェックだらけになることは明白です。
本来登録処理をしたいregisterメソッドが責務を持ちすぎる、という問題に直面します。

表現力が乏しい

Map<String, Integer> studentsScore

「studentsScore」のkeyはなんでしょうか？
なんとなく「教科ID」ではないかと想像できます。
が、おそらく皆さんは想像通り本当にkeyが「教科ID」であるか確認しにコードを読みに行くでしょう。
…コードの推測はバグになりやすいですし、なによりめんどくさくない？

変更が大変

「おっ！こっちのサービスクラスでも教科ごとの成績を使いたかったんや！コードコピペして完了！」
…地獄の保守開発の始まりです。
システムが高校のテストにも対応することになったため、点数の上限を「200」へ変更することになりました。
「教科ごとの点数」を扱っている箇所をすべて洗い出すためにGrepをかけます。
もしかしたらコピペしてない「教科ごとの点数」が存在するかもしれないので目視で確認もし…
＼(^o^)／

パラメータを間違える可能性がある

studentGradeService.Register("すばらしい👌", 
                             "student123",
                             Map.of("Math", 95, "Science", 88));

パラメータのStudentIdとremarksはどちらもString型です。
すなわち、パラメータを間違えて指定したとてもIDEさんやコンパイラさんは怒ってくれません。
レビューや単体テストで上司さんが怒ってくれるでしょう。

どうしたか

値やコレクションを扱うクラスを作成しました。

/**
 * 備考（0～100文字）を表すクラス
 */
public class Remarks {
    /** 備考の値 */
    private final String value;

    /**
     * プライベートコンストラクタ。直接生成は不可。
     * @param value 備考の値
     */
    private Remarks(String value) {
        this.value = value;
    }

    /**
     * 備考の値を取得する
     * @return 備考の値
     */
    public String getValue() {
        return value;
    }

    /**
     * 備考値オブジェクトを生成するファクトリメソッド
     * @param value 備考の値（1～100文字）
     * @return Remarksインスタンス
     * @throws IllegalArgumentException 値がnull/空、または100文字を超える場合
     */
    public static Remarks of(String value) {
        // 備考がnullまたは空の場合は例外
        if (value == null || value.isEmpty()) {
            throw new IllegalArgumentException("Remarks cannot be null or empty");
        }
        // 備考が100文字を超える場合は例外
        if (value.length() > 100) {
            throw new IllegalArgumentException("Remarks must not exceed 100 characters");
        }
        return new Remarks(value);
    }
}

public class SubjectScores {
    /** 科目IDと点数のマップ */
    private final Map<SubjectId, Score> values;

    /**
     * プライベートコンストラクタ。直接生成は不可。
     * @param rawScores 科目IDと点数のマップ
     */
    private SubjectScores(Map<SubjectId, Score> rawScores) {
        this.values = rawScores;
    }

    /**
     * ファクトリメソッド。バリデーションを行いインスタンスを生成する。
     * @param scores 科目IDと点数のマップ
     * @return SubjectScoresインスタンス
     * @throws IllegalArgumentException scoresがnull、または不正な点数が含まれる場合
     */
    public static SubjectScores of(Map<SubjectId, Score> scores) {
        // マップがnullの場合は例外
        if (scores == null) {
            throw new IllegalArgumentException("Scores must not be null");
        }
        // いずれかの点数が0未満または100を超える場合は例外
        if (scores.values().stream().anyMatch(score -> score.getValue() < 0 || score.getValue() > 100)) {
            throw new IllegalArgumentException("Scores must be between 0 and 100");
        }
        return new SubjectScores(scores);
    }

    /**
     * 指定した科目IDの点数を取得する
     * @param subjectId 科目ID
     * @return 指定科目の点数（存在しない場合はnull）
     */
    public Score getScore(SubjectId subjectId) {
        return values.get(subjectId);
    }

    /**
     * 全科目の点数マップを取得する（変更不可のMapを返す）
     * @return 全科目の点数マップ
     */
    public Map<SubjectId, Score> getValues() {
        return Collections.unmodifiableMap(values);
    }

    public void Register(StudentId studentId, Remarks remarks, StudentScores studentsScores) {
        // ここで登録処理を行う
    }

こうすることで、「問題」はすべて解消されます。

さいごに

今回の開発では増田亨さんの「現場で役立つシステム設計の原則」を大いに参考にさせていただきました。
「業務ロジックがどこに書いてあるかを特定しやすく、業務ルールの変更が必要になったときに変更の対処箇所を限定し、変更の影響を狭い範囲に閉じ込めることがオブジェクト指向で業務アプリケーションを開発する狙い」というお言葉は金言であり、今後のシステム開発でも意識して業務に取り組みたいです。