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

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

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

今回作ったQiitaWidgetの要件

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

レビュー時点での設計

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

おおまかな処理の流れは

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

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

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

レビューによる指摘

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

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

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

CachedApiConfCreator.ts

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内へ記事配列のソートやシャッフルといった機能を実装してありました。

QiitaPresenter.ts

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クラス内で行っていました。

QiitaWidget.ts

// 記事がなくなるか、最大試行回数に到達するまでリクエストを続ける
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;
}