ワンライナーをリーダブルにリファクタリング

今回はコードレビュー中にリーダブルコードを思い出すことがあったのでその事例を紹介します。

前提として以下のコードになります。

Webブラウザで動作する
Babel でトランスパイルして minify する

とあるプロジェクトで以下の様な JavaScript のコードがありました。
(コンテキストが分かりやすいようにプロパティ名等は変更してます)

reviewerAttributeNames() {
  return this.currentProject ? [...Array(this.currentProject.max_reviewers).keys()].map(n => ++n).map(n => `reviewer${n}`) : []
},

よく読めば何をしているのかは分かる気がしますが、読むのに少し時間がかかります。

例えば this.currentProject.max_reviewers の値が 3 だとしましょう。
このメソッドの戻り値はどうなるでしょうか？

結果はこんな感じです.

[
  "reviewer1",
  "reviewer2",
  "reviewer3"
]

何となくは結果は想像できますが、途中の過程のコードが読みづらいと思います。

リファクタリング

リファクタリング

さて本題です。
これをリファクタリングしていきます。
細かく見ると色々ありますが、主要な注目すべき以下の点があります。

三項演算子を使っている
Array.prototype.map() メソッドが２度呼ばれている

[ガード節] Guard Clauses を使う

まずは三項演算子をなんとかしていきましょう。
ここではリーダブルコードの 7.5　関数から早く返す で書かれている、ガード節 (Guard Clauses) を使います。
ついでにメソッドチェーンをしてるところに改行も入れていきます。

reviewerAttributeNames() {
  if (!this.currentProject) {
    return [];
  }

  return [...Array(this.currentProject.max_reviewers).keys()]
    .map(n => ++n)
    .map(n => `reviewer${n}`);
},

ガード節のおかげで条件分岐自体はシンプルになりました。
元のコードよりは断然マシです。

ワンライナーを分解

ここでもリーダブルコードの 8.4　短絡評価の悪用 で書かれている. １行で書かれているコードを分解するテクニックを使います。

鍵となる考え

「頭がいい」コードに気を付ける。あとで他の人がコードを読むときにわかりにくくなる。

スプレッド構文やらArray.prototype.keys()やArray.prototype.map()は忘れて普通に書いていきます。


reviewerAttributeNames() {
  if (!this.currentProject) {
    return [];
  }

  const { max_reviewers } = this.currentProject;
  const values = [];
  for (let i = 1; i <= max_reviewers; i++) {
    values.push(`reviewer${i}`);
  }
  return values;
},

愚直ですがfor文を使うのがベターです。

関数化する

レビュー当時はこの1箇所しかありませんでしたが複数箇所あるのであれば関数を用意するのもいいと思います.

lodashのtimesを使うのもいいですし、下記のような関数を用意しても良いです.

function times(n, iter) {
  const values = [];
  for (let i = 0; i < n; i++) {
    values.push(iter(i));
  }
  return values;
}

上記のような関数を用意していればかなりシンプルになります.

reviewerAttributeNames() {
  if (!this.currentProject) {
    return [];
  }

  const { max_reviewers } = this.currentProject;
  return times(max_reviewers, (i) => `reviewer${i + 1}`);
},

それでもワンライナーが使いたいと思う人へ

ワンライナーでも少しはリーダブルにはなります。
例えば以下のように書き直すこともできます。

reviewerAttributeNames() {
  if (!this.currentProject) {
    return [];
  }

  const { max_reviewers } = this.currentProject;
  return Array(max_reviewers).fill('reviewer').map((v, i) => `${v}${i + 1}`);
},

ここで前提を思い出してみましょう。