I have some logic on my JSX for an element in a grid, if element is selected, then add to redux, if element is not selected then remove from redux...

is working ok, but I sense a bad smell...

          onChange={(categoryId, isSelected) => {
            favorites[categoryId] = isSelected;
            const favsArraya = Object.values(onboarding.favoriteCategories);
            let index;

              if (favsArraya.indexOf(categoryId) === -1){
                //doesnt exist, add it
              if (favsArraya.indexOf(categoryId) !== -1){
                // exists, delete it
                index = favsArraya.indexOf(categoryId);
                favsArraya.splice(index, 1);

            dispatchData({ [DATA_KEY.FAVORITE_CATEGORIES]: favsArraya})

  • \$\begingroup\$ I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate. \$\endgroup\$ Jan 14 at 19:31

First of all, there's way too much going on here for an inline function. Let's seperate that out. Also, the splicing can be replaced with the more expressive filter.

const addIfMissing = (array, item) =>
    array.includes(item) ? array : array.concat([item]);

const without = (array, exclude) =>
    array.filter(x=> x!==exclude);

const onGridSelectorChange = (categoryId, isSelected) => {
    favorites[categoryId] = isSelected;
    const categoryIds = Object.values(onboarding.favoriteCategories);
    const newCategoryIds = isSelected ?
        addIfMissing(categoryIds,categoryId) :

    dispatchData({ [DATA_KEY.FAVORITE_CATEGORIES]: newCategoryIds})

  • \$\begingroup\$ thanks a lot, what is the name of that kind of inline function writing? functional programming? reactive programming? \$\endgroup\$ Jan 14 at 23:13
  • \$\begingroup\$ the inline function is a JS arrow function. It's useful for functional programming, but does not necessarily imply "pure" functional code \$\endgroup\$ Jan 14 at 23:32

Why is index declared at the top of the onChange handler? It is only used within the else block once. Thus it could be declared there using const.

const index = favsArraya.indexOf(categoryId);
favsArraya.splice(index, 1);

Not only does this eliminate an extra empty declaration, using const is a good habit because it can help avoid accidental re-assignment and other bugs.

And that variable can be eliminated since it is only used once:

favsArraya.splice(favsArraya.indexOf(categoryId), 1);
  • \$\begingroup\$ I don't believe the conditionals can be combined. Consider the case where isSelected=false and categoryId does not exist within favsArraya \$\endgroup\$ Jan 14 at 19:26
  • 1
    \$\begingroup\$ Okay that makes sense- thanks for clarifying. \$\endgroup\$ Jan 14 at 19:28

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.