同僚のコードレビューでこんなにクラスの設計が良くなったという話

弊社では、案件とは関係のないプロジェクトでも業務時間中にみんなにコードレビューを依頼できる時間が確保されています(参加は任意)。案件のコードレビューを依頼したり、ちょっとした個人の制作物を見てらったりと使い方は色々です。

先日、TypeScriptの練習にQiitaのAPIを叩いていて記事を表示するブログウィジェットを作成しました。このウィジェットのレビューを依頼したところ、クラスの設計について具体的な指摘と、それに対する改善を経験できたのでこの記事に記載します。

今回作ったQiitaWidgetの要件

• Qiitaの公式APIV2から記事とユーザー情報を取得し、HTMLテンプレートに表示する
• 投稿の合計いいね数を算出するために、あるユーザーの投稿を全件取得する
  • (このために複数回リクエストの送信とレスポンスデータの結合を行う)
• パラメータによってユーザー、いいね数によるソート、表示件数、ランダム表示の有無などをカスタマイズできる
• ajaxライブラリにaxiosを使う／jQueryを使わない
• APIの制限を回避するために、レスポンスデータをブラウザにキャッシュする機能がある
• TypeScriptで実装する
  • any型の使用は最小限にする

レビュー時点での設計

レビュー時点でのクラスの設計をゆるく図にまとめました。

おおまかな処理の流れは

1. QiitaWidgetがCachedApiConfCreatorでGETのためのパラメータを作成
2. CachedApiからCachedResponseの型でレスポンスを受け取る
3. QiitaPresenterへレスポンスを渡して描画してもらう

……というものです。実装の詳細はGitHubの過去履歴を参照してください。

• qiita-widget-js/src/lib at 0.0.3 · hokkey/qiita-widget-js

レビューによる指摘

• 防御的な引数の型チェックが多いが、TypeScriptならもっと引数を信頼していい
• QiitaPresenterはビューの表示に特化し、データの整形などを行うべきではない
• キャッシュ機能を自前で実装しているが、既存のライブラリがあるのでそれを使った方がよい
  • さらに、CachedApiという名前から職務が想像しにくい
• QiitaWidget内にAPI絡みの詳細な実装を入れない方が良い

防御的な引数の型チェックが多いが、TypeScriptならもっと引数を信頼していい

普段JavaScriptで実装する際、パラメータに想定外のデータ型が入るのが嫌で型チェックをかける癖がありました。

static validateConf(conf: CachedApiConf): void {
  if (!isType(conf.id, 'string')) {
    throw new TypeError('id must be a string!');
  }

  if (!isType(conf.endpoint, 'string')) {
    throw new TypeError('endpoint must be a string!');
  }

  if (!isType(conf.axiosRequestConfig.params.page, 'number')) {
    throw new TypeError('page param is not available!');
  }

  if (!isType(conf.axiosRequestConfig.params.per_page, 'number')) {
    throw new TypeError('per_page param is not available!');
  }
}

• qiita-widget-js/CachedApiConfCreator.ts at 0.0.3 · hokkey/qiita-widget-js

しかし、TypeScriptであれば引数の型はインタフェースで保証できるため、コンパイルが通っているのであれば余計な型チェックは不要でした。ブラウザ実行時に想定と異なる型で値が渡る可能性のあるところにだけ型チェックがあれば良いと思います。

QiitaPresenterはビューの表示に特化し、データの整形を行うべきではない

QiitaPresenterは、受け取ったレスポンンスデータをビューに展開する責務を負ったクラスとして設計しました。「表示のための整形であればQiitaPresenter内で処理するべきかな?」という気持ちでQiitaPresenter内へ記事配列のソートやシャッフルといった機能を実装してありました。

private createTargetArticleList<T>(source: T[], orders: number[]): T[] {
  // Sort the list if not it using shuffle
  let articles = (this.conf.sortByLike && !this.conf.useShuffle)
    ? Util.sortArray(source, 'likes_count')
    : source
  ;

  // Create a list of required articles
  articles = orders.map((val) => {
    return source[val];
  });

  // Sort the list if it using shuffle
  if (this.conf.sortByLike && this.conf.useShuffle) {
    return Util.sortArray(articles, 'likes_count');
  }

  return articles;
}

• qiita-widget-js/QiitaPresenter.ts at 0.0.3 · hokkey/qiita-widget-js

しかし、QiitaPresenterはビューの表示に特化するべきで、データ自体を加工するのは外部で行うべきという指摘を受けました。なるべくクラス1つあたりの責務を小さく設定するのがベターということでした。

キャッシュ機能を自前で実装しているが、既存のライブラリがあるのでそれを使った方がよい

axiosそれ自体にはキャッシュ機能がありません。そのため、レスポンスデータを日付付きでlocalStorageへ保存したり読み込んだりするCachedResponseというクラスを作っていました。

• qiita-widget-js/CachedResponse.ts at 0.0.3 · hokkey/qiita-widget-js

しかし、レビュアーからは「ググったら既存のライブラリがあったからそれ使ったら?」という指摘が……。たしかに、axios自体にキャッシュ機能はありませんでしたが、axiosのAdapterとしてaxios-cache-adapterというライブラリが公開されていました。

• axios-cache-adapter

どう考えても自分の雑な実装よりも信頼性に優れていて実装も隠蔽できるため、そちらを採用するように変更しました。

教訓としては一般的な機能は既存のライブラリがないかまず探してみるということに尽きると思います。

QiitaWidget内にAPI絡みの詳細な実装を入れない方が良い

CachedApiクラスは汎用を意図していたため、次ページのリクエストや、複数ページのレスポンスの結合といった処理はQiitaWidgetクラス内で行っていました。

// 記事がなくなるか、最大試行回数に到達するまでリクエストを続ける
private async fetchList(): Promise<void> {
  let counter = 0;
  while (counter < this.conf.maxRequest) {
    if (await this.fetchOnce()) {
      counter++;
      continue;
    }
    break;
  }
  return;
}

private async fetchOnce(): Promise<boolean> {
  const res: CachedResponse<QiitaResponse.Article[]> =
    await CachedApi.get<QiitaResponse.Article[]>(this.apiConfCreator.getNextConf());

  // 記事数0：ループ終了
  if (res.data.length === 0) {
    return false;
  }

  // 記事数がperPage未満：結果を追加してループ終了
  if (res.data.length < this.conf.perPage) {
    this.dataList = this.dataList.concat(res);
    return false;
  }

  // 記事数がperPageと同値以上：結果を追加してループ継続
  this.dataList = this.dataList.concat(res);
  return true;
}

• qiita-widget-js/QiitaWidget.ts at 0.0.3 · hokkey/qiita-widget-js

レビュアーからは「APIとの通信 + 全件取る処理を他へ分けた方が良い」、「fetchOnceが副作用とbooleanの戻り値の両方の仕事を持っているのが良くない」「fetchOnceという名前も微妙」という指摘を受けました。

APIとの通信 + 全件取る処理を他へ分けた方が良い

これが問題としては一番大きく、QiitaWidgetはマネージャー的な役割に徹し、詳細な実装はQiitaWidgetの外側に出した方が良いということでした。

fetchOnceが副作用とbooleanの戻り値の両方の仕事を持っているのが良くない
fetchOnceという名前も微妙

もっともですが実装中には気付けませんでした。こういった客観的な指摘が得られるのがコードレビューの良いところだと思います。

改善結果

型チェックの除去

防御的な引数の型チェックが多いが、TypeScriptならもっと引数を信頼していい

引数の型はインタフェースで定義されているため、型チェックを外しました。ただし、数値の最大値・最小値といった内容の検証は残している箇所があります。

axios-cache-adapterの導入

キャッシュ機能を自前で実装しているが、既存のライブラリがあるのでそれを使った方がよい

キャッシュ機能を外部ライブラリで賄えるようになったため、CachedApi、CachedResponseといったクラスは廃止しました。これによりコード内でキャッシュを気にすることが完全に不要となりました。

QiitaWidgetとQiitaPresenterの機能を新クラスへ分離

QiitaPresenterはビューの表示に特化し、データの整形を行うべきではない

旧QiitaWidgetの処理の一部と、旧QiitaPresenterの行っていたデータ操作的な処理を新たにQiitaItemsとしてまとめなおしました。QiitaItemsの責務はレスポンスデータのリクエスト／格納と、QiitaPresenter用のデータの整形／出力です。

• qiita-widget-js/QiitaItems.ts at 0.3.2 · hokkey/qiita-widget-js

これにより、QiitaPresenterはデータの表示に特化し、QiitaWidgetは直接レスポンスデータを操作する必要がなくなりました。

QiitaWidget内にAPI絡みの詳細な実装を入れない方が良い

旧QiitaWidgetと旧CachedApiParamCreatorの機能を統合したQiitaItemsApiを新たに作成しました。責務はデータの取得と、GETパラメータの管理機能です。

• qiita-widget-js/QiitaItemsApi.ts at 0.3.2 · hokkey/qiita-widget-js

前項の変更と併せ、QiitaWidgetの責務が非常にシンプルになりました。

• qiita-widget-js/QiitaWidget.ts at 0.3.2 · hokkey/qiita-widget-js

fetchOnceが副作用とbooleanの戻り値の両方の仕事を持っているのが良くない

こちらは、booleanを返す機能と、レスポンスをプロパティに追加する機能にきっちり分けました。

改善後の設計

全体的にクラスの責務がより細分化され、役割がはっきりした設計となりました。

• qiita-widget-js/lib at 0.3.2 · hokkey/qiita-widget-js