little hands' lab

ドメイン駆動設計を布教したい

【コード問題集1】責務違反のRepository

今日の記事では、 「こんなコードを書くとあとでびっくりするからやめた方がいいよ!」 という事例をご紹介します。

問題のあるコード

以下のようなタスクEntityがあったとします。
コンストラクタで生成時のルールを,doneメソッドで状態遷移のルールを表現しています。

public enum TaskStatus {
    UNDONE, DONE
}

class Task {
    private String taskId;
    private String name;
    private TaskStatus taskStatus;
    private LocalDate dueDate;

    public Task(String name, LocalDate dueDate) {
        this.name = name;
        this.dueDate = dueDate;
        // idは初期化時にUUIDで生成される
        this.taskId = UUID.randomUUID().toString();
        // ステータスは自動で「未完了」が設定される
        this.taskStatus = TaskStatus.UNDONE;
    }

    // setterではなく、完了時の制御用のメソッドのみ提供
    public void done() {
        this.taskStatus = TaskStatus.DONE;
    }

    // emit getter
}

それに対して、以下のようなUseCaseクラスを作成しました。
名前と期日は渡されたパラメーター通り、IDとステータスはEntityの中でルールに基づき設定される、シンプルな処理です。

class TaskUseCase {
    @Autowired
    private TaskRepository taskRepository;

    public String createTask(String name, LocalDate dueDate) {
        // 初期化時のステータスはコンストラクタで設定されて「UNDONE」になる
        Task task = new Task(name, dueDate);
        taskRepository.save(task);
        return task.getTaskId();
    }
}

それに対してテストを書きます。

class TaskUseCaseTest {
    @Autowired
    private TaskAppService taskAppService;
    @Autowired
    private TaskRepository taskRepository;

    void save_pastDate() {
        // when: 3日前の期日でタスクを作成
        String savedTaskId = 
           taskAppService.createTask("taskName", LocalDate.now().minusDays(3));

        // then: ステータスが未完了、期日が3日前で保存される
        Task savedTask = taskRepository.findById(savedTaskId);
        assertThat(savedTask.getDueDate(),
               is(LocalDate.now().minusDays(3)));
        assertThat(savedTask.getTaskStatus(),
               is(TaskStatus.UNDONE)); // → ERROR!!
    }
}

これは当然通・・・ると思ったら、最後のアサーションで以下のようなエラーが発生しました!!

java.lang.AssertionError: 
Expected: is <UNDONE>
     but: was <DONE>

そんな馬鹿な!!コンストラクタで確実に UNDONE を設定しているはずなのに!!

・・・さて、原因はなんでしょう?









・・・おやおや、コードを追ったらこんなところにこんな処理が。

class TaskRepository {
    @Autowired
    private JdbcTemplate jdbc;

    public void save(Task task) {
        if (task.getDueDate().isBefore(LocalDate.now())) {
            // 過去日だったら完了したことにする
            task.done();
        }
        String sql = "INSERT INTO tasks ('task_id','name','task_status', 'due_date') VALUES (?,?,?,? )";
        Object[] params = {task.getTaskId(), task.getName(), task.getTaskStatus().toString(), task.getDueDate()};
        jdbc.update(sql, params);
    }
   //
}

こらー!

repositoryが勝手に
「過去日だったら完了したことにす」なーーーー!!!

何が問題だったか

このコードは、Repositoryの責務違反ということができるでしょう。

Repositoryの責務は集約単位でドメインオブジェクトと永続化層の変換を行うことであり、それ以外の処理を行うことは責務違反です。

また、save というメソッド名でsave以外のことをしているので、驚き最小の原則 (システムのコンポーネントは、その利用者の期待する形で動作する必要がある)に違反します。

コードの使用元が想定したものとは異なる挙動を実装することは、予期せぬバグの原因になりますし、あとでコードを追う時にも原因を見つけづらい、可読性を下げる原因になります。

解決策

まず、Repositoryは永続化の責務以外のことをしないようにします。

Repositoryの説明として、よく「RepositoryはListのように使う」という説明をします。List<User> usersがあったとして、
user.add したものを users.get した結果、値が変わっていたらどうでしょうか? 混乱すると思います。

また、 users.add(user)users.remove(users)はありますが、 users.registerUser(name, address)
users.deactivate(userId)とはしません。

なので、Repository#saveでは、永続化のみするようにします。

また、過去日だったら完了したことにするに関しては、意図が不明確なので本来どうあるべきだったのか?をきちんと定義するところから始めましょう。

タスク生成時に過去日を指定したらエラーにしたければ、Entityのコンストラクタに書くと良いでしょう。過去日のものをまとめて完了にしたいというユースケースがあるならば、それはUseCaseクラスで実現しましょう。

まとめ

「責務違反」はかなりコードの色々なところで発生しがちです。
そもそも責務を意識しないコードを書いてしまうことが多いです。

責務を考えるとこ、つまりこのクラスは何をするクラスなのか?はとても重要なので、常に考えていきましょう。