はじめに

あくまで「自分ならこう書くかな」というお気持ち表明程度の文章であり、元記事のコードを批判する意図は一切ありません🙏

あらかじめご了承下さいますようお願い致します。

本題

ある日チームにテストコードを書く文化を定着させるを読んでいると気になるコードが出てきた。

// 異常系パターン１: 返ってくるPromiseが失敗すると異常系になる。
await this.feelingRankingRepository.checkLatestRankingDataAndGetRecordCount();
// 異常系パターン２: count=0だとエラーになる。
if (count === 0) {
   this.logger.error(`今週の対象データはまだ公開されていません。確認の上、再処理をお願いします。`);
   throw Error(`No data to publish this week in FeelingRanking`);
 }
// 異常系パターン３: 返ってくるPromiseが失敗すると異常系になる。
await this.feelingRankingRepository.publish();

自分のRepository Patternに対する理解は以下の通り。

整合性を保ちたい単位(集約)で永続化するためのデザインパターン
一部の項目(プロパティ)だけを更新するようなメソッドが生えるのはルール違反
- articleRepository.update(title: 'xxx') など
- 更新処理はdomain層に移譲すべき、でないとデータの不整合が発生する可能性がある

先ほど引用したコードを例に話すと、Repositoryがあるのでそれに対応する集約があると考えられる。今回の場合feelingRankingRepositoryなのでFeelingRankingという集約があるとイメージして疑似コードを書いてみた。

import { FeelingRankingRepository } from "@/repositories/feelingRanking";

@injectable()
export class FeelingRankingService {
  constructor(
    @inject(DiTokens.Logger)
    private logger: Logger,
    @inject(DiTokens.Repositories.FeelingRanking)
    private feelingRankingRepository: FeelingRankingRepository,
  ) {}

/**
 * ランキングデータを公開する
 * @returns {Promise}
 **/
public async publish(id: number): Promise<void> {
  this.logger.debug(`FeelingRanking Publish: start`);

  const feelingRanking = await this.feelingRankingRepository.getLatest();
  feelingRanking.publish(); // 実装イメージは後述

  try {
    await this.feelingRankingRepository.update(feelingRanking);
    this.logger.debug(`FeelingRanking Publish: end`);
  } catch (e) {
    this.logger.error(`FeelingRanking Publish error: ${e}`);
    throw e;
  }
}

ポイントは

const count = await this.feelingRankingRepository.checkLatestRankingDataAndGetRecordCount();

を

const feelingRanking = await this.feelingRankingRepository.getLatest();

に変更したところ。

Repositoryは集約単位で保存して集約単位で取得するのが原則なので checkLatestRankingDataAndGetRecordCount のようなメソッドは違和感がある。

せめて getLatest メソッドで取得した上で集約に自分が何件のデータを持っているのかを把握するプロパティを生やす方が良いと考えました。(個人的には getLatest も納得がいっていません)

またメソッドに and が入るのは異なる処理が入っている証拠なので、分割できないかを考えた方が良いかなと思いました。

そしてFeelingRanking集約は以下の通り。

class FeelingRanking extends Aggregate {
  // constructorとか色々あって...
  
  public publish(): void {
    if (this.count === 0) throw Error(`No data to publish this week in FeelingRanking`);
    this.published = true;
  }
}

この実装方法だと記事で紹介されていたコードと比較すると、publishの処理がドメイン層に移動したことで単体テストが可能になりモックを挟む必要がなくなったのでよりテストの信頼性が増している(ハズ)。