あるドメイン層のPRでレビューコメント久しぶりに沢山きた

振り返り

インプットの記録

あるPRの振り返り。結構質問のコメントも多く、そこは自分がdescriptionに書いていなかった情報の不足なので反省している。PRで指摘されたことは以後繰り返したくないので、PRのオープン時間が長いものに関してはPRをオープンにする前からクローズ(merge)されるまでの流れを振り返ることにする

やったこと

ドメイン層の実装
ドメイン層のテスト

もらったレビューから思うこと・再発防止

ドメイン実装時はファクトリメソッドをどこに置くかを意識しろ

そもそもファクトリメソッドがやりたいことは

インスタンスとして生成されるクラス(ドメインオブジェクト)がある時、インスタンスを生成するクラスをファクトリとして定義する
- ドメインオブジェクトのインスタンスを生成したい場合は、必ずファクトリでドメインオブジェクトのインスタンスを生成する
インスタンスの生成をファクトリに任せるようにして、利用者側がドメインオブジェクトのインスタンス生成の具体的手順がわからなくても良いようにする

って感じ。例えばAAAというドメインオブジェクトに対し、以下を作成する

data class AAA(
  val hoge: Hoge,
  val fuga: Fuga
){
  companion object {
    fun of(
        hoge: Hoge,
        fuga: Fuga
    ): AAA {
      return AAA(
        Hoge.new,
        Fuga.new
        )
     }

ここでいう、AAA.of がファクトリメソッドにあたる。

学んだこととしては、純粋な生成ロジックだと判断できるものはそのクラスのファクトリメソッドにしてあげたほうがクラスとその振る舞いの紐付きが明確になるのでこれがまさにドメイン駆動かといった感じ。ドメインは外側(アプリケーションサービス)に依存しないように、あるべき振る舞いのみを書いて、アプリケーションサービスではその型を当てはめてあげるだけな感じ。

わかってても気づかないことが多いんだよな。ドメイン外にあるドメインの生成メソッドを書いてたりした。AI駆動でやってる僕が悪いんだろうな。。。w

テストに実装の詳細を持ち込むな/依存度を高くするな

めちゃくちゃ良いレビューをもらった。元々は

value class TestUUID(val value: UUID)

を持っていて、ドメイン内やテストでインスタンス化する時は

// テストコード
val testUUID = TestUUID(randomUUID)

みたいにしていた。しかし、第三者からすると、テスト内で上記を見た時、「randomUUID??? これはjava製の内製関数？？それとも我々が内製したもの？」となるが、以下のように定義すれば、テストではそういった誤解を与えない。

value class TestUUID(val value: UUID){
  companion object {
    fun new() = TestUUID(randomUUID)
  } 
}

// テストコード
val testUUID = TestUUID.new()

これの何が嬉しいかというと、testUUIDはTestUUIDの値オブジェクトを生成しているんだろうと開発者が直感的に思うことと、純粋な生成に特化していて内部で何をしているかが隠蔽されていること。テストには

「テストは実装の詳細に依存するべきではない」

という原則があり、まさにこのことかと思った。要はなんの処理をモックしているかが直感的にわかりやすく、実装の詳細に依存していないのでテストが純粋で読みやすいコードになるんだよな。コメントくれた天才プログラマ感謝。

コレクションは雰囲気で使うな

kotlinはArrayの種類として、List,Map,Setの3種類がある。違いは以下。

List

順序あり / 重複可。使い所は、順序付きのデータを管理したいかつ重複を許容するとき。

Set

順序なし・重複不可。使い所は、一意な値の集合を扱いたいとき。

Map

キーと値(key/value)。ペア・キーは一意。例えば、以下の場合は最後のものだけが有効になる

val map = mapOf(
    "id" to 1,
    "name" to "Alice",
    "id" to 2
)

println(map) // => {id=2, name=Alice}

つまりキーは一意であり、重複したら上書きされる。使い所は、キーによって値を管理したいとき。

僕はよく雰囲気でとりまListかMapを使うことが多いが、ListとSetの区別は意思意図を持てるようにしたい。

data class系のテストはインスタンス丸ごとアサートせよ

プロパティ単位ではなくインスタンス丸ごとアサートした方がよい。プロパティ増えても変更に強い。とはいえ、それは知っていたが、何気考えたことなかったのが、プロパティ単位にした方がいい時とインスタンス丸ごとにした方がいい時とで区別してなかったなと。これはメンターに来週聞きたい。

ハードコーディングを避けろ

// 悪い例
data class SampleData(
    val userId: UserId,
    val userName: UserName,
    val email: Email,
    val phoneNumber: PhoneNumber?
) {
    fun toMap() = mapOf(
        "userId" to userId.value,
        "userName" to userName.value,
        "email" to email.value,
        "phoneNumber" to phoneNumber?.value
    )
}

// 良い例
data class SampleData(
    val userId: UserId,
    val userName: UserName,
    val email: Email,
    val phoneNumber: PhoneNumber?
) {
    fun toMap() = mapOf(
        this::userId.name to userId.value,
        this::userName.name to userName.value,
        this::email.name to email.value,
        this::phoneNumber.name to phoneNumber?.value
    )
}

toMapメソッドの中身を、前者はハードコーディング(型推論がない)、後者は型推論がある。みたいなイメージ。要はtypoとかのリスクを防げるんですよね。後者はリファクタリングに強い。シンプルに知らなかった。

まだまだ知らないことばかりだなああ。でもほんとにレビューが丁寧で嬉しい。こんだけたくさん言われると、まだまだなんだなって思う。一つずつ潰していきたい