2
\$\begingroup\$

I have a transformer helper function. It reduces over the array and transform key/value pairs. At the end of the loop there is the key 'EXAMPLE1' exists and I should insert two objects after the first and the second array(result) items. Right now, it works as expected, but I have concerns - is it a good approach for using splice inside reduce in my case?

 const commonData = Object.entries(settings).reduce((result, [currentKey, currentValue]) => {
        if (currentKey === SETTINGS_FIELDS.EXAMPLE1) {
            const type = {
                key: myKey,
                value: myValue,
            }
            const code = {
                key: myKey,
                value: myValue,
            }

            const insertedData = result.splice(2, 0, type, code)

            return [...result, ...insertedData]
        }



        return result.concat({ key: transformToUpperCase(currentKey), value: currentValue })
    }, [])

    return [...commonData]
}
```
\$\endgroup\$
1
  • \$\begingroup\$ I don't understand why you're using reduce here. Do you just want to insert these two key/value pairs into the array? Then just use splice() on the array directly. Do you want to do it only if the settings object contained the key EXAMPLE1? Then do it. What do you mean by "at the end of the loop"? \$\endgroup\$ – JollyJoker Dec 27 '19 at 16:52
1
\$\begingroup\$

Looks like you were trying to follow the best practices on not mutating data but this is an overkill:

  • when processing data locally in a function there's no need to clone the intermediate data
  • no need to clone the array in return, it's already a temporary cloned array
  • reduce is the wrong choice here - at least if you keep its needless duplication of the resultant array in each step - the task is a simple transformation of each element so map() seems a better choice + an additional splice on the special key
  • insertedData is always an empty array so no need for [...result, ...insertedData]
  • for consistency, if you use splice and concat then go all the way and use concat instead of array spread as well or switch to array spread instead of concat.

Simplified code using map():

function foo() {

  const commonData = Object.entries(settings).map(([key, value]) => ({
    key: transformToUpperCase(key),
    value,
  }));

  if (Object.hasOwnProperty.call(settings, SETTINGS_FIELDS.EXAMPLE1)) {
    commonData.splice(2, 0, {
      key: myKey,
      value: myValue,
    }, {
      key: myKey,
      value: myValue,
    });
  }

  return commonData;
}

If you don't override prototype of settings or objects in general the key check can be simplified further: if (SETTINGS_FIELDS.EXAMPLE1 in settings)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.