js ループ処理の最適化

Question

下記コードを 最適化したい

課題

const list = []; と return list; が見苦しい

_.map() をうまく利用したいが方法が思いつかない

const list = {
  'sports': [
    {
      identify: 'soccer',
      name: 'サッカー',
    },
    {
      identify: 'baseball',
      name: '野球',
    },
  ],
  'nature': [
    {
      identify: 'animal',
      name: '動物',
    },
    {
      identify: 'flower',
      name: '花',
    },
  ],
};

const groups = [
  {identify:'sports', id: 1},
  {identify:'nature', id: 2},
];

const make = (tmpList, groups) => {
  const list = [];
  _.forEach(groups, group => {    
    _.forEach(tmpList[group.identify], v => {
      v['group_id'] = group.id;
      list.push(v);
    });
  });

  return list;
};

console.log(
  make(list, groups)
);

Comments

[defghi1977]

引数tmpList, groupsの形状と, 関数makeがどのような結果をもたらす(もたらしたい)のかについて記述を追加して下さい.

[tommyTeratail]

データを準備しました。 動作は期待通りですが makeが見苦しいのでアドバイスが欲しいです

[turbgraphics200]

ほんとに期待通りなのでしょうか？、提示されたコードを実行すると返されるlistのgroup_idがすべて2になるのですが。

[tommyTeratail]

おっしゃる通りですね、 バグです

[tommyTeratail]

修正しました。

Answer 1

修正後のコードも、その場しのぎの修正のような気がしてならないのですが、makeが返すlistの値として期待するものは(可読性度外視, Chrome/Firefoxのみ)

const make = (t, g) => g.reduce((a, {identify, id}) => !t[identify].map(i => a[a.push({...i}) - 1].group_id = id) || a, []);  

の結果だと思われますが。

Comments

[tommyTeratail]

修正後のコードは、正しいです

回答ありがとうございます。
期待通りの動作です。
魔術のようなコードですね。
私には難しかったです

Answer 2

2次元配列にしてからflattenで1次元配列に叩き落とすか、
reduceで2次元配列→1次元配列に戻しながら格納するかの二択ですね。

こんな感じでいかが？

const make = (tmpList, groups) =>
  _(tmpList)
    .map(tmp =>
      _.map(groups, group => ({...tmp, id: group.id}))
    )
    .flatten()
    .values()

const users = [
  { user: 'barney', age: 36, active: true },
  { user: 'fred',  age: 40, active: false },
  { user: 'travis', age: 37, active: true}
];
const ids = [
  { id: 123 },
  { id: 234 }
];
console.log(make(users, ids));
// いい感じにでました

---

【おまけ】

reduce使うルートならネイティブJSでも出来そうなのでやりました。

const make = (tmpList, groups) =>
  tmpList
    .map(tmp => groups.map(group => ({...tmp, id: group.id})))
    .reduce((arr, items) => {
      items.forEach(it => { arr.push(it) });
      return arr;
    }, [])

const users = [
  { user: 'barney', age: 36, active: true },
  { user: 'fred',  age: 40, active: false },
  { user: 'travis', age: 37, active: true}
];
const ids = [
  { id: 123 },
  { id: 234 }
];
console.log(make(users, ids));
// こちらもいい感じの結果に

---

【追記】 グループ分けしたオブジェクトへの対応版

条件変わってますので再度考え直してみます。
まぁ、オブジェクトはtoPairsを使って配列にしてしまえば消化試合です。

キーに引っかかるグループが存在しなかった場合を考慮して、
存在しないグループは一度filterで消し飛ばすというガードも組み込みました。

const list = {
  'sports': [
    {identify: 'soccer', name: 'サッカー'},
    {identify: 'baseball', name: '野球'}
  ],
  'nature': [
    {identify: 'animal', name: '動物'},
    {identify: 'flower', name: '花'}
  ]
};
const groups = [
  {identify:'sports', id: 1},
  {identify:'nature', id: 2},
];

const make = (tmpList, groups) =>
  _(tmpList)
    .toPairs()
    .filter(([gname]) => _.find(groups, group => group.identify === gname))
    .map(([gname, items]) => {
      const group = _.find(groups, group => group.identify === gname);
      return _.map(items, it => ({...it, group_id: group.id}));
    })
    .flatten()
    .value()

result = make(list, groups)

https://codepen.io/travist/full/jrBjBz/にコピペすればちゃんと表示されることがわかります。
結果:

[
   {
      "identify": "soccer",
      "name": "サッカー",
      "group_id": 1
   },
   {
      "identify": "baseball",
      "name": "野球",
      "group_id": 1
   },
   {
      "identify": "animal",
      "name": "動物",
      "group_id": 2
   },
   {
      "identify": "flower",
      "name": "花",
      "group_id": 2
   }
]

Comments

[miyabi-sun]

うぅ、いけると思って未テストだったから動かなかった・・・
修正して動作確認しました

[miyabi-sun]

ちな、このコードだと名前がbarney、fred、fravisのデータが2件ずつ出来てしまいますが大丈夫ですか？
しかもID重複してるので正常なデータには思えませんが…
tmpListだけに単なるテストデータ？

[tommyTeratail]

サンプルコードありがとうございます。
確かに動かないですが...

flattenで動作確認してみます

https://codepen.io/andreic/pen/xpLBz

[tommyTeratail]

提示していただいたコードが、理想の形式です

[miyabi-sun]

修正後コードだとこの手は使えないですね、
対応版のコード作るのでちょっと待ってくださいね

[tommyTeratail]

回答ありがとうございます

途中で条件が変わってしまいすみません。
検証が甘いコードをはっていました。
何度も回答本当にありがとうございます

期待通りの回答です。

pushしてreturnするのは、jsらしくない感じと思っていました。
新しい考え方を得た気がします

toPairsばっちりですね

[tommyTeratail]

おまけのほうもありがとうございます

Answer 3

回答者はLodashに詳しくありません.

---

JavaScriptの標準APIを使うとこんな感じでしょうか?
tmpList内のオブジェクトが出力結果配列内に何度も現れるのが気になりますが.

const tmpList = [{v: 1}, {v: 2}];
const groups = [{id: 1}, {id: 2}];
const make = (tmpList, groups) => groups.reduce(
	(prev, curr) => prev.concat(...tmpList.map(v => (v.id = curr.id, v))), //vをクローンしたほうが良いのかも
	[]
);
console.log(make(tmpList, groups));

Comments

[defghi1977]

LodashのforEachはオブジェクトを受けるので, これだと駄目だね

[tommyTeratail]

回答ありがとうございます。

> vをクローンしたほうが良いのかも
可能であれば理由がしりたいです。

[defghi1977]

誤った前提でのコード・コメントなので, お気になさらず. （言外に元のコードバグってね？という意図がありました）

[tommyTeratail]

質問したコードが検証甘かったのでご迷惑おかけしました。
協力ありがとうございます。