スマートな配列処理をしたい

Question

実現したいこと

お店idを格納した配列と、お店の情報が格納された配列があります。

const shopIds = [1, 2];
const shops = [
    { id: 1, name: "テラ貴族", isOpen: true},
    { id: 2, name: "テラサカバ", isOpen: true},
    { id: 3, name: "テラ民", isOpen: true},
    { id: 4, name: "テラ木屋", isOpen: true},
];

配列shopIdsに格納されたidを持つshopのisOpenフラグをfalseに書き換えるプログラムを書きました。

shopIds.forEach(id => {
  shops.forEach(shop => {
    if (shop.id === id) {
      shop.isOpen = false;
    }
  });
});

現在、2重でループさせることでフラグを書き換えていますが、これをmapやfilterなどのメソッドを使ってスマートに書く方法を模索しています。ご教授いただけますと幸いです。

Comments

[m.ts10806]

falseではなくtrueになってますが・・

[os1_nmnm]

ご指摘ありがとうございます。修正いたしました。

[miyabi-sun]

> 配列shopIdsに格納されたidを持つshopのisOpenフラグをfalseに書き換えるプログラムを書きました。
ふんふん……
> shop.isOpen = true;
書けてないやん

[miyabi-sun]

ｂ

Answer 1

constで作ったオブジェクトに対して破壊的操作で値をいじるのはあまり良くありません。
という訳で両方のケースで書きます。

今回の質問文には書かれてませんが、
多分このshopIdsは何らかの事情によりWebサイト上で一時的に準備中にしたいという目的で定義された変数だろうと推測されます。
なのでそれ想定で書いてます。

---

プロパティの値に代入はオブジェクトの破壊的な操作になります。
今回のような破壊的操作を伴う場合、戻り値は不要なのでmapは使わずforEachを使うようにしてください。
mapを使うと未来の貴方や、他のメンバーは「戻り値を使って何かしたいんだ」と勘違いする要因になるので決して使わないでください。

また、こういう2つの配列を使う場合はどちらを起点にするかでコードの長さが変わります。
両方書いてみてしっくり来る方を選びましょう。

shopIdsを先に使うとこんな感じのコードになります。

shopIds.forEach(id =>
  (shops.find(it => it.id === id) || {}).isOpen = false
);
console.log(shops);
// (4) [{…}, {…}, {…}, {…}]
// 0: {id: 1, name: "テラ貴族", isOpen: false}
// 1: {id: 2, name: "テラサカバ", isOpen: false}
// 2: {id: 3, name: "テラ民", isOpen: true}
// 3: {id: 4, name: "テラ木屋", isOpen: true}

findの結果で見つからない場合undefinedになり、undefinedのプロパティを操作しようとするとエラーが出ます。
or演算子で空オブジェクトを生成して返すようにすることでプロパティの操作が出来るようにしました。
showIdsが常に存在するIDならば決め打ちでいけるのでもう少しかわいいコードになります

shopIds.forEach(id =>
  shops.find(it => it.id === id).isOpen = false
);

// あるか無いか分からない場面だと先にfilter使うんだけど、ちょっと冗長かなぁ？
shopIds
  .filter(id => shops.some(it => it.id === id))
  .forEach(id => shops.find(it => it.id === id).isOpen = false)

逆にshopsを使えばこのようなコードになるでしょう。

shops.forEach(it => {
  if (shopIds.includes(it.id)) it.isOpen = false
})
console.log(shops);
// (4) [{…}, {…}, {…}, {…}]
// 0: {id: 1, name: "テラ貴族", isOpen: false}
// 1: {id: 2, name: "テラサカバ", isOpen: false}
// 2: {id: 3, name: "テラ民", isOpen: true}
// 3: {id: 4, name: "テラ木屋", isOpen: true}

// こちらもfilter版、かなりシンプルでスマート！
shops
  .filter(it => shopIds.includes(it.id))
  .forEach(it => it.isOpen = false)

---

オブジェクトの破壊的操作を避け、コピーする想定でいくならこんな感じになります。
JavaScriptのプロパティ値に代入する方法は破壊的操作なので使えません。
mapとシャロコピーを併用する方式になります。

shopIdsを主軸にすると変更＋変更してないショップ一覧にたどり着くのが困難になりますので、
shops.mapを主軸にするのが適切でしょう。

const initialisedShops = shops.map(it => ({
  ...it, isOpen: shopIds.includes(it.id) ? false : it.isOpen
}));
console.log(initialisedShops);
// (4) [{…}, {…}, {…}, {…}]
// 0: {id: 1, name: "テラ貴族", isOpen: false}
// 1: {id: 2, name: "テラサカバ", isOpen: false}
// 2: {id: 3, name: "テラ民", isOpen: true}
// 3: {id: 4, name: "テラ木屋", isOpen: true}

Answer 2

こうでしょうか。

shops.map(shop => shopIds.includes(shop.id) ? {...shop, isOpen: false} : shop);

includes, Object.assign についてはご自身で確認してください。

Answer 3

jsは参照渡しているのでmapで返す必要はないですね

const shopIds = [1, 2];
const shops = [
    { id: 1, name: "テラ貴族", isOpen: true},
    { id: 2, name: "テラサカバ", isOpen: true},
    { id: 3, name: "テラ民", isOpen: true},
    { id: 4, name: "テラ木屋", isOpen: true},
];
shops.forEach(function(x){
  x.isOpen=!shopIds.some(function(y){return y=== x.id;});
});
console.log(shops);

もう少し複雑な条件になった場合も想定するとfilterしてからforEachするという考え方もあります

shops.filter(function(x){
  return shopIds.some(function(y){return y=== x.id;});
}).forEach(function(x){
  x.isOpen=false;
});

訂正版

含まれない場合はそのままという処理としました

const shopIds = [1,2];
const shops = [
    { id: 1, name: "テラ貴族", isOpen: null},
    { id: 2, name: "テラサカバ", isOpen: true},
    { id: 3, name: "テラ民", isOpen: null},
    { id: 4, name: "テラ木屋", isOpen: true},
];
shops.forEach(function(x){
  if(shopIds.some(function(y){return y=== x.id;})){
    x.isOpen=false;
  }
});
console.log(shops);

Comments

[miyabi-sun]

前半のコードがroot_jpさんの所にも書いた件と同様で、要件満たしてなさそうです。

[yambejp]

たしかに

[miyabi-sun]

対応ありがとうございます！

[yambejp]

いえとんでもない、ご指摘ありがとうございます

Answer 4

const result = shops.map(shop => {
  shop.isOpen = !shopIds.includes(shop.id);
  return shop;
});
console.log(result);
/*
 * 0: Object { id: 1, name: "テラ貴族", isOpen: false }
 * ​1: Object { id: 2, name: "テラサカバ", isOpen: false }
 * ​2: Object { id: 3, name: "テラ民", isOpen: true }
 * ​3: Object { id: 4, name: "テラ木屋", isOpen: true }
 */

shopIdsに含まれていない場合にはisOpenを更新しない版

var result = shops.map(shop => {
  if (shopIds.includes(shop.id)) shop.isOpen = false;
  return shop;
});

Comments

[miyabi-sun]

> 配列shopIdsに格納されたidを持つshopのisOpenフラグをfalseに書き換えるプログラムを書きました。

これだとidを持たない場合はisOpenがtrueに書き換わっているので要件を満たしていませんね。
質問文自体の日本語がダメで実はこれが正解の可能性もありますが、コードも違うと言ってるように見えます。

また、オブジェクトのプロパティを触っている為破壊的変更になっておりshopsの中身が書き換わってます。
これでresult変数を宣言してmapを使っていることから、
他のエンジニアは「ああ、ちゃんとシャロコーピー使ってるから大丈夫なんだな」と勘違いする要因になり危険です。

変数宣言＋mapの方針で行くのであれば、7Kreuzさんの回答のようにしっかりシャローコピーを掛けた方が良いでしょう。

[root_jp]

> idを持たない場合はisOpenがtrueに書き換わっている
これはダメですね。。。

constのオブジェクトのデータを書き換えてるのも良くないです。
今回のオブジェクトは全てプリミティブ型で構成されているので、
確かにシャローコピーすれば十分そうですね。
この辺りの副作用は気を付けなければならないなぁ。。。

Answer 5

こんなんでどうでしょう

const shops2 = shops.map(shop => ({
  ...shop, isOpen: !shopIds.includes(shop.id)
}))

Comments

[miyabi-sun]

root_jpさんの所にも書いた件と同様で、要件満たしてなさそうです。
コード自体はとても洗練されてて好きです。

[7Kreuz]

あ、確かにそうですね＞要件
ご指摘ありがとうございます