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

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

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

• 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}`);
},

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

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

ショートコーディングのために工夫したはずですが、 minify までするとこの差はほとんどないと思います.

ちょっと試し書きする感じだと重宝するワンライナーですが、基本的には使わないほうが読みやすくなります.