# 2021-07-15 メンターセッション

## 質問

### 特大課題コードレビュー（粟田）

#### 良いと思ったところ
- UniqueEntityIDに_kazznameがある
- ただプロパティじゃなく型レベルで書いた方が良さそう
- UseCaseでOutputDataのクラスを用意しているところ
- Entity、VOの抽象クラスは評判が良さげ
- AggregateRootを用意したのは未来が良さそう

#### 気になったところ
- UseCaseのI/Fはあっても良いがそこまで依存性逆転するわけではないので効果は薄い
- 防御的プログラミングをするなら、ユースケースの入力データにもバリデーションを書いても良さそう（例えば空文字チェックとか）
- より早い段階で弾くことができるので、パフォーマンス的なメリットが生まれる
- saveメソッドがvoid
- saveに渡すオブジェクトを返しても良い
- factoryだけにバリデーションをまとめるのは良くなさそう
- ドメインオブジェクトに振る舞いを持たせた方が柔軟になる
- ペアから参加者を削除するときだけ動かすバリデーションとか
- ディレクトリ構成はほぼ満点
- テストは__tests__にまとめるなどするとよかったかも？
- プラハでは`domain/infra/`のなかにリポジトリのインターフェース入れてる
- create/restore
- createRandom/createとかでもいいかも？（好みの問題）

## 松原さんの方で事前に用意していただいたメモ
### 良い！
- DIコンテナまで作ってるところが素晴らしい
- Id voのklazznameが素晴らしい。プラハではこんな感じで定義してる。抽象化した親の方でprivatePropertyを持っておけば、klazz設定忘れを防げるかも

export default class UserId extends ValueObject<string, 'UserId'> {
  public constructor(value: string) {
    super(value)
    if (isEmpty(value)) {
      throw new Error('user id must be not empty')
    }
  }
}

export default abstract class ValueObject<T extends string | number | boolean, U extends string> {
  private _: U
  public readonly value: T
  public constructor(value: T) {
    this.value = value
    this._ = '' as U
  }
  // ValueObject同士の比較にはequalsを使用するか、value同士の比較を行う
  // ValueObject同士の比較に === を使用した場合はfalseになるため注意が必要
  public equals(obj: ValueObject<T, U>): boolean {
    return this.value === obj.value
  }
}

- CreatePairOutputDataって形でユースケースから出ていく値も制御しているのがとても良い
- Entityを作ってるのも素晴らしい（abstractの中でequalsを定義している）
- isAlreadyBelongToPairとか、ロジックに名前をつけて分離しているのがgood

### 気になったので聞いてみた点
- Controllerの中でinput Dataを作成している背景（無効な値があった時のハンドリングロジックとかはコントローラーでやるのかな？）
- もしする場合、primitiveな値のままusecaseに渡して、usecaseの中で確認した方が良いことがあるかもしれない
- Create-pair-usecaseのsaveでentityを返していないのはどうしてだろう？（再取得していたので気になった）
- 保存に使ったentityをそのまま返しちゃっても良いかも？と思った
- 例外が発生していないのにおかしな値がDBに保存されるのはテストで担保すべきことなのかな？と（アプリケーションの実行時にはそこを信頼しても良いのかな？と思った）
- どうしてaggregateRootのabstractを作成したんだろう？
- remove-participant-usecase、一般的にはpairをrestoreして「removeParticipant」することの方が多いと思う
- 現状はusecaseにドメインロジックが染み出してしまう可能性がある
- そうしないと厳密にremoveParticipantした時のテストが書けない（factoryに一人削除した引数を渡すことは可能だけど、これだと「少ない人数の初期化」をテストしている状態なので）
- ペアに2人しかいない場合のバリデーションってどこで実施してるんだろう？（削除しちゃいけないはずなんだけど）

###### Tags: `Team-2`