「奥野さんと社員のリファクタリング部屋」は、リファクタリングに励むトレタの社員と技術顧問の奥野さん ( @okunokentaro ) の間で実際に行われた会話を切り取った開発現場実録コンテンツです。

技術顧問: 奥野さん
三度の飯よりリファクタリングが好き。

今回の質問者: 武市さん
トレタ在籍２年。沖縄在住のフロントエンジニア

今回の質問

今回は初期リリースを終えたWebアプリケーション(Next.js)のプロダクトを担当している武市さんから、複数人で開発を進めてきて統率が取れなくなったディレクトリ構造のリファクタリングについての質問です。

APIで外部とやり取りしている部分をリファクタリングして、クリーンアーキテクチャに沿ってリポジトリを作ろうと考えています。

その中で、GraphQL APIレスポンスの結果を変換するアダプター関数(adaptGetIServiceItemsAggregateResult)はどこのディレクトリに置くのがいいですか？

下記はGraphQLリクエストを行うリポジトリの関数です。

src/server/boundary/repository/service/serviceRepository.ts

export const createServiceRepository = (
  gqlClient: GqlClientType,
  client: HasuraClient,
): ServiceRepository => {
  return {
    updateServiceWithExternalKeys: async (
      id: string,
      updateData: UpdateData,
      externalKeys: Record<string, string>,
    ): Promise<UpdateServiceWithExternalKeysResult> => {
      const mutationDocument = makeServiceMutationDocument(externalKeys);
      const result = await client.request<
        UpdateServiceResult,
        UpdateServiceVariables
      >(mutationDocument, {
        id,
        name: {
          ja_JP: updateData.name,
        },
        displayName: {
          ja_JP: updateData.displayName,
        },
        printName: {
          ja_JP: updateData.printName,
        },
      });
      return adaptUpdateServiceWithExternalKeys(result);
    },
    getIServiceItemsAggregate: async (
      itemManagementGroupId,
      name,
    ): Promise<GetIServiceItemsAggregateResult> => {
      const result = await gqlClient.getIServiceItemsAggregate({
        itemManagementGroupId,
        name: {
          ja_JP: name,
        },
      });
      return adaptGetIServiceItemsAggregateResult(result);
    },
  };
};

「単一責任の原則」から外れていないか

奥野

createServiceRepository()という関数の返り値がupdateServiceWithExternalKeys() とgetIServiceItemsAggregate() という関数を持つオブジェクトになっているのはどういった意図ですか？

武市

サービスリポジトリの中でサービスに関わることをすべてやってしまいたいと思ったからです。

奥野

それはgetIServiceItemsAggregate() という単独の関数でもよさそうにみえました。わざわざcreateServiceRepository() という関数でオブジェクトを返さなくてもいいかもしれない。

見たところ、引数のgqlClient とhasuraClient は異なるユースケースのためのクライアントのようです。この2引数を両方必要とする関数がありません。つまりこのリポジトリは「単一責任の原則」から外れていて、テストする時にどちらか必要のないモックも作る必要があるんですよ。

つまり、getIServiceItemsAggregate() のテストをしたいだけなのにupdateServiceWithExternalKeys() の準備もしないとテストが始められない状況ってことです。

サービスリポジトリはオブジェクト定数ではなく、あくまでもディレクトリとしてrepository/service/getIServiceItemsAggregate.ts を作るといいと思います。

export const getIServiceItemsAggregate = async (
  gqlClient,
  itemManagementGroupId,
  name,
): Promise<GetIServiceItemsAggregateResult> => {
  const result = await gqlClient.getIServiceItemsAggregate({
    itemManagementGroupId,
    name: {
      ja_JP: name,
    },
  });
  return adaptGetIServiceItemsAggregateResult(result);
};

武市

なるほど。

奥野

今って処理をなるべく細かくしたいという段階ですね。細かくすればするほど一つのテストがしやすくなるためそうしようというモチベーションなのに、これでは小さくしたものをくっつけてまた大きくしちゃってます。

武市

たしかに。リポジトリというものに全部詰め込んだ方が、と思ったんですが。

奥野

リポジトリというのはあくまでも考え方で、外部のシステムに依存するものがリポジトリという場所にまとまっているといいよね、という考え方です。

まず、リポジトリ層というものがどういう経緯で生まれたかという歴史的背景をわかっていた方がいいと思います。

リポジトリ層はもともとJavaなどで取り入れられたクラスベースの考え方として広まっています。Javaはクラスしか扱えないからこそ、リポジトリクラスにインスタンスメソッドを生やす、必然的にコンストラクタに初期値を渡す、すなわちそこで依存性を備えるということをせざるを得なかったんです。

我々はTypeScriptプログラミングをしているので、必ずしもJavaの作法を真似する必要はなく1関数1ファイルというTypeScriptなりの書き方をすることもできます。

なので、repository/service/ というリポジトリのディレクトリの中にそれぞれの関数ファイルを作るという方法もありますよ。

repository/
    ├ service/
        ├ updateServiceWithExternalKeys.ts
        ├ getIServiceItemsAggregate.ts

細かく分けたいなら、今はとにかく細かく分けることに注力するといいと思います。

1責務 1 関数1ファイル

奥野

ファイルを分割することで、アダプタをどこ置くかという問題は自然に求まります。

関数を細かく分ければ分けるほどアダプターはそこでしか使わなくなる。なのでディレクトリをさらに細かくしていって、そこにアダプター関数を置けばいいということになります。

repository/
    ├ service/
        ├ updateServiceWithExternalKeys/
            ├ updateServiceWithExternalKeys.ts
            ├ adaptUpdateServiceWithExternalKeys.ts
        ├ getIServiceItemsAggregate/
            ├ getIServiceItemsAggregate.ts
            ├ adaptGetIServiceItemsAggregateResult.ts

これでなにが嬉しいかというと、adapt関数にそんな長い名前を付ける必要があるかと、いう問題の解決となる。

これまでなぜ名前を長くしないといけなかったかというと、今まで同じファイルに同居しており区別するための名前を付ける必要があったから。同じファイルにいないのであればadaptResult()で十分ですね。

もうちょっと言うと updateServiceWithExternalKeys.tsっていうのもupdate.tsで良いし、getIServiceItemsAggregate.tsもgetAggregate.tsで十分です。IServiceItemsという名前はそれ自体がちょっと怪しいですね…。

repository/
    ├ service/
        ├ updateServiceWithExternalKeys/
            ├ update.ts
            ├ adaptResult.ts
        ├ getIServiceItemsAggregate/
            ├ getAggregate.ts
            ├ adaptResult.ts

もし武市さんが名前つけていて「この名前ダサいな」と思ったってことは何か良くないこと、微妙なことやってる証拠なんですよ。名前がおかしいってことは、そこで扱っている粒度がおかしいっていうことですね。

武市

疑問を持つタイミングが違ってました。リポジトリを作る際にgraphqlClinetとhasuraClientを注入しないといけない段階でキモいなと思ってたんですよ。

奥野

「キモい」という感覚はすごくいい一歩で、やっぱりキモいものって引数構成とか戻り値構成でどこか間違えてるんです。

オブジェクト指向を踏襲するとか、Dependency Injectionの考え方を踏襲するっていうのはすごくいい考え方ではあるんだけど、それは全てSOLID原則のS（単一責務の原則）を守ってこそ。そういう各原則とかを守れてないあやふやなオブジェクトの状態であれこれ考えても、元が整っていないとツラいですね。 1責務 1 関数1ファイルという粒度にどんどん削ぎ落としていく。そうあるべき姿に持っていけば、リポジトリという巨大なオブジェクトは作る必要がなくなります。

武市

なるほど。先ほどキモいという言葉使ったんですが「違和感」というものもうちょっと大事にしていこうと思います。

依存性注入とカリー化

奥野

getIServiceItemsAggregate()という関数を単独で切り出すと、引数がgqlClient itemMangementGroupId name の3つになりますが、もとのcreateServiceRepository() で gqlClientの部分をわけていたのは、パラメーターではなくDependency Injection をやろうとしたということですよね。そこを分離させることは賛成で、gqlClientもパラメーターっぽく見えるのは嫌ですよね。そこで使うのがカリー化です。

武市

依存だけ先に引数で渡すという話ですか？

奥野

その通り。TypeScriptで依存性注入する時はclass constructorでやるかカリー化した関数の引数でやるかが多くて、それをうまくできるような関数設計に寄せていくと綺麗にまとまっていきます。

type GetIServiceItemsAggregateResult = {}; // ダミー

type Return = (
  itemManagementGroupId: string,
  name: string,
) => Promise<GetIServiceItemsAggregateResult>;

export const getAggregate = (gqlClient: GqlClientType): Return => {
  return async (itemManagementGroupId, name) => {
    const result = await gqlClient.getIServiceItemsAggregate({
      itemManagementGroupId,
      name: {
        ja_JP: name,
      },
    });
    return adaptResult(result);
  };
};

type UpdateServiceWithExternalKeysResult = string;

type Return = (
  id: string,
  updateData: UpdateDate,
  externalKeys: Record<string, string>,
) => Promise<UpdateServiceWithExternalKeysResult>;

export const update = (client: HasuraClient): Return => {
  return async (id, updateData, externalKeys) => {
    const mutationDocument = makeServiceMutationDocument(externalKeys);
    const result = await client.request<
      UpdateServiceResult,
      UpdateServiceVariables
    >(mutationDocument, {
      id,
      name: {
        ja_JP: updateData.name,
      },
      displayName: {
        ja_JP: updateData.displayName,
      },
      printName: {
        ja_JP: updateData.printName,
      },
    });
    return adaptUpdateServiceWithExternalKeys(result);
  };
};

名前を短くして見通しやすくする

奥野

関数名を短くしたら全体的に名前を短くする工夫もするといいですね。

makeServierMutationDocument()という名前も、もしこの関数だけでしか使わないのだったら makeDocument()にしたらいいし、idがstring型ではなくItemManagementGroupId 型ってわかれば別に変数名として長々と付ける必要はないから idで十分とか、読みやすくできる部分はどんどん読みやすくしていくと、さっきより読みやすくなった。

type GetIServiceItemsAggregateResult = {};
type Return = (
  id: ItemManagementGroupId,
  name: string,
) => Promise<GetIServiceItemsAggregateResult>;

export const getAggregate = (gqlClient: GqlClientType): Return => {
  return async (id, name) => {
    const result = await gqlClient.getIServiceItemsAggregate({
      itemManagementGroupId: id,
      name: { ja_JP: name },
    });
    return adaptResult(result);
  };
};

type ServiceId = string;
type Return = (
  id: ItemManagementGroupId,
  updateData: UpdateDate,
  externalKeys: Record<string, string>,
) => Promise<ServiceId>;

const update = async (client: HasuraClient): Return => {
  return async (id, updateData, externalKeys) => {
    const doc = makeDocument(externalKeys);
    const variables = {
      id,
      name: { ja_JP: updateData.name },
      displayName: { ja_JP: updateData.displayName },
      printName: { ja_JP: updateData.printName },
    } satisfies UpdateServiceVariables;
    const result = await client.request<
      UpdateServiceResult,
      UpdateServiceVariables
    >(doc, variables);
    return adaptResult(result);
  };
};

奥野

長い名前ってことは長く説明しないと伝わらない概念ってことであって、それは概念整理できてないってことなんですよ。概念整理ができると、開発者同士で前提の共有ができるため短い名前でも通じるんですよね。

なのでなぜupdate()の一言で済むかというと、パスがrepository/service/updateだからなんですよ。/serviceの中にいるアップデート関数だからサービスのアップデートなんだってわかる。

それはコンテキストが補えてるんですよね。

でもそれがコンテキストがないとupdateServiceWithExternalKeys()って全部の要素を名前の上で言わないと伝わらないし、名前以外の情報が足りてないってことだとわかります。だから repository/serviceいうディレクトリを切ったおかげでそれが何かっていうのがわかるようになるんです。

武市

ありがとうございます！僕もちょっと違和感があったところが解決したのでありがたいです！