食べチョク開発者ブログ

食べチョクエンジニアによるプロダクト開発ブログ

map,filter,reduce関数内で状態を書き換えてはいけないのは、なぜですか

皆さんこんにちは、エンジニアの西尾です。

あなたは今、コードレビューをしています。

以下コードに直面したとき、何を指摘しますか。 修正してほしい点を、どのようにレビュイーに伝えますか。

// これはJavaScriptのコードです。
// 商品の在庫を1つ減らし、売り切れになったものを抽出したい、と思っています。

const soldOutProducts = products.filter(product => {
  product.quantity -= 1;

  return product.quantity <= 0;
});

よくないコードレビューの例

問題は表題の通り、filterの中で状態を書き換えているのが、よくありません。

関数型言語を学んだことがある方なら、このコードの違和感に気がつきます。 filterは純粋関数であるべきだ、副作用を起こしてはいけない。そう認識しているからです。

しかし、それをコードレビューで指摘したとして、相手に伝わるでしょうか。 書いている言語は関数型ではないし、副作用とか言われても意味がわからないし、複雑なコードでもないし、動くからいいんじゃないの。

map,filter,reduce内で状態を書き換えるコードは、思いの外よく見かけます。 そのたびに、私は以下のような指摘をしてしまっていました。

f:id:vividgarden-tech:20210123215718p:plain

f:id:vividgarden-tech:20210123215721p:plain

f:id:vividgarden-tech:20210123215725p:plain

問題は何なのか?

filter内で状態を書き換えてはいけません、と伝えるだけでは、レビューを受けた側も意味がわかりません。 このコードが良くない理由を説明しないと、相手には伝わりません。

なぜ状態を書き換えてはいけないのか。 言われてみると、私自身もすんなり理由を説明できませんでした。

そこで、自分なりにこのコードの問題点を、今一度考えてみました。

複数の目的をもったコードは、わかりづらい

UNIXの基本的な考えに、「ひとつのプログラムには、ひとつのことをうまくやらせる」というものがあります。 目的を最小限に抑えた小さなプログラムは、誰にとってもわかりやすく、保守も容易です。

プログラムに限らず、この考えは1つの関数・1つのブロック・1行のコードにも当てはまります。 複数の目的を持った処理は理解しづらいものです。

今回の例では、「在庫を減らす」操作と「売り切れの商品を抽出する」操作が同時に行われているため、 複雑なコードに見えます。

本来の目的と違う使い方をしているから、わかりづらい

filter関数は、与えられた条件に当てはまるデータを抽出(フィルタリング)するために利用します1。 「フィルターをする」処理をお願いしたのに、在庫を減らす処理も同時にされてしまうのは、驚きがあるロジックです。

mapやreduceも同様で、「渡されたリストを別のものにマッピングする」「渡されたリストを畳み込む」以外の挙動をさせるのは、理解しづらいコードです。

広い範囲に影響を及ぼすロジックは、わかりづらい

呼び出すたびに中身が書き変えられるロジックは、わかりにくいです。

例えば、次のようなメソッド呼び出しで中身も書き換えられてしまったら、直感に反する理解しづらいコードではないでしょうか。

console.log(product); // { name: "商品1", quantity: 3 }

const quantity = getQuantity(product);

console.log(product); // { name: "商品1", quantity: 2 } !? まさか中身が書き換わっているとは
function getQuantity(product) {
  product.quantity -= 1; // 渡された引数の中身を破壊している

  return product.quantity;
}

これは極端な例ですが、今回の例にあるfilterの使い方も、同じようなわかりづらさがあります。

じゃあ、どうすればいいの?

どのように修正すればよいでしょうか。 コード断片だけを見て、適切なアドバイスをするのは難しいです。 在庫を減らすという処理は、別のところで、あらかじめしておくべきでしょうか。設計から見直す必要があるかもしれません。

それでも直すとしたら、以下のようになるでしょうか。

products.forEach(product => {
  // どうしても値を書き換えたかったら、eachを利用する
  product.quantity -= 1;
});

const soldOutProducts = products.filter(product => product.quantity <= 0);

値を書き換える必要があるのなら、forやforEachを使います。 もちろん、書き換えずにすむなら、それに越したことはありません。

おわりに

map,filter,reduce関数内で値を書き換えてしまう違和感を、言語化してみました。 これらの関数をfor(each)と同じ感覚で使ってしまう方が、案外多いように思えます。

今回紹介したメソッドは、あくまで一例です。 例えば、RubyのEnumerableにあるようなメソッドで値を書き換えるのは、避けるべき実装です。

この問題は、コードレビューの時点で、私自身もすんなり理由を回答できませんでした。 あたりまえだと思っていることを見直すことで、自分自身の理解もより一層深めることができました。

www.wantedly.com

www.wantedly.com


  1. 余談ですが、この記事を書くためにリーダブルコードを読み返していました。第3章に、filterという名前は「選択する」のか「除外する」のかあいまいだから避けるべき、との指摘をみつけて、確かに!と納得しました。ただ、JavaScriptにselectはないので、しょうがない。