クラスを作る時はクラス使用者がメソッドの呼び出し時に誤解しないように設計しよう

これは何?

最近、インスタンス変数を使うか関数の引数を使うかで迷いました。そこでインターネットで調べて出てきた伊藤さん @jnchito
のブログが参考になりました。

自分はこれを読んで，
「クラスの使用者がメソッドの呼び出しを誤解しないのが良い設計」
だと理解したのですが，実際にそれをコードに反映させてみるといい感じになるまで，結構試行錯誤したのでその過程をこの記事では共有します。

異論反論を待っていますので，良い設計をご存知の方はぜひコメントいただけると助かります。
TypeScript/JavaScriptにも不慣れなのでこのあたりでも助かります。

---

前提としての自分の思想

私は自分がコードを書くときには「何をやっているかが見えやすいコードを書く」という考え方を重視しています。

そのため，細かくメソッドとファイルをわけることを意識してコードを書いています。↓イメージ

index.ts

import { MCPClient } from "./mcp_client";
import { getAnthropicApiKey, parseMCPJson, getServerNames } from "./config";

async function main() {
  const apiKey = getAnthropicApiKey();
  const mcpJsonPath = "../mcpservers.json";
  const mcpJson = parseMCPJson(mcpJsonPath);
  const serverNames = getServerNames(mcpJson);
  const serverName = serverNames[0]; // TODO: 一旦1つのサーバのみで動作するようにする

  const mcpClient = new MCPClient(apiKey);
  try {
    await mcpClient.initialConnect(mcpJson, serverName);
    const userMessage = await mcpClient.getUserMessage();
    await mcpClient.callAnthropicApi(userMessage);
  } finally {
    await mcpClient.cleanUp();
  }
}

main();

ですが，伊藤さんの記事を読み，クラスを使用する人が誤解しにくいかという観点が不足していたことに気づきました。具体的には以下の部分です。MCPClientをnewしたあとにinitialConnectが必要なことを呼び出し側で誤解する可能性があると思います。

index.ts

  const mcpClient = new MCPClient(apiKey);
  try {
    await mcpClient.initialConnect(mcpJson, serverName);
    const userMessage = await mcpClient.getUserMessage();
    await mcpClient.callAnthropicApi(userMessage);
  } finally {
    await mcpClient.cleanUp();
  }

mcpLcinet.getUserMessage()
で取得したユーザからの入力をcallAnthropicApi()で使用しているのでこの部分は順序が逆になったり使用されない可能性は低いのではと考えています。

---

改善案1: initialConnect()をインスタンス作成時にまとめて行えるようにする(非採用)

クラス呼び出し側を以下になるように修正します。

index.ts

  // 静的ファクトリーメソッドを使って接続済みのMCPClientを取得
  const mcpClient = await MCPClient.connect(apiKey, mcpJson, serverName);
  
  try {
    const userMessage = await mcpClient.getUserMessage();
    await mcpClient.callAnthropicApi(userMessage);
  } finally {
    await mcpClient.cleanUp();
  }

クラス側のコンストラクタで単純にinitialConnect()を呼ぶと非同期処理の完了を待つメソッドが必要になりそうで普通に使いにくいなと思ったので，ファクトリメソッドを使うことにしました。
↓ファクトリメソッドを使わない例では，waitForConnection()が増えてしまった。

index.ts

  const mcpClient = new MCPClient(apiKey, mcpJson, serverName);

  try {
    // Wait for connection to complete
    await mcpClient.waitForConnection();
    const userMessage = await mcpClient.getUserMessage();
    await mcpClient.callAnthropicApi(userMessage);
  } finally {
    await mcpClient.cleanUp();
  }

解決案1に対する自分の感想

ファクトリメソッドを使った影響でクラス側のコードが煩雑になりました。
クラスの使用者としては誤解しにくくなりましたが，開発者としてはシンプルなコードの方がいいはずです。
そのため，最終的に自分は以下に記載する解決案2を採用することにしました。

---

解決案2 インスタンス変数を使う代わりにinitialConnect()の戻り値を使う(採用)

callAnthoropicApi()に引数としてuserMessage，toolsを渡す必要があるので必然的にそれらを取得するメソッドを先に呼び出す必要があるとクラスの使用者は認識するはずです。

index.ts

  const mcpClient = new MCPClient(apiKey);
  try {
    const tools = await mcpClient.initialConnect(mcpJson, serverName);
    const userMessage = await mcpClient.getUserMessage();
    await mcpClient.callAnthropicApi(userMessage, tools);
  } finally {
    await mcpClient.cleanUp();
  }

これであれば，クラス側の実装もシンプルな状態に保つことができ，リファクタリングの範囲を絞ることができました。

また副次的にですが，initialConnect()の中でMCP Serverで使用可能なツールの一覧を取得していることがクラスの実装を読まなくてもわかるようになりました。

詳細を隠蔽すべきという考え方はあるものの，initialConnect()のテストもやりやすくなったので，自分はこの変更で良いと思っています。
もともとのテストでは，インスタンス変数に対するgetterを作ってわざわざツールの一覧が更新されていることを確認していましたが，今後はgetterが不要になりました。

mcp_clinet.test.ts

// もとのテスト
describe("initialConnect", () => {
  test("MCPサーバーに接続し，ツールリストを取得する", async () => {
    const apiKey = "test_api_key";
    const mcpClient = new MCPClient(apiKey);
    await mcpClient.initialConnect(mockMcpJson, "github");
    expect(mcpClient.getTools()).toBeDefined();
});

mcp_client.test.ts

// リファクタリング後のテスト
describe("initialConnect", () => {
  test("MCPサーバーに接続し，ツールリストを取得する", async () => {
    const apiKey = "test_api_key";
    const mcpClient = new MCPClient(apiKey);
    const tools = await mcpClient.initialConnect(mockMcpJson, "github");
    expect(tools).not.toEqual([]);
});

---

まとめと感想

• クラスを設計する時には，クラス使用者が誤解しないような書き方を意識すると良い。
• 今回はインスタンス変数をやめてプライベート変数を使うことで，最小限のリファクタリングで上記の課題を解決できた。
• 思わぬ誤算としてテストもいい感じになった。
• AI Agentを使って今回リファクタリングを行ったため，自分は設計に集中できたのが良かった。ジュニアはある程度まできたら設計に集中すべきなのかもしれない

---

補足

複数MCPサーバを使用するような改修を予定している場合，以下のようにやるよりもtoolsをインスタンス変数にしてこのあたりの詳細を隠蔽した方がシンプルなのかなと思ったりしました。
自分は上の方が好みではありますが人によっては下の方が好みかも。

index.ts

  const mcpClient = new MCPClient(apiKey);
  try {
    const tools1 = await mcpClient.initialConnect(mcpJson, serverName1);
    const tools2 = await mcpClient.initialConnect(mcpJson, serverName2);
    // toosl1とtools2を統合
    const mergedTools = tools1.concat(tools2);
    const userMessage = await mcpClient.getUserMessage();
    await mcpClient.callAnthropicApi(userMessage, mergedTools);
  } finally {
    await mcpClient.cleanUp();
  }

  // インスタンス変数を使うコード
  const mcpClient = new MCPClient(apiKey);
  try {
    await mcpClient.initialConnect(mcpJson, serverName1);
    await mcpClient.initialConnect(mcpJson, serverName2);
    const userMessage = await mcpClient.getUserMessage();
    await mcpClient.callAnthropicApi(userMessage); // toolsはクラスのインスタンス変数から取得して使用する。
  } finally {
    await mcpClient.cleanUp();
  }

---

全体のソースコードを見たい方へ

自分のリポジトリ