2
\$\begingroup\$

I have the following object at React state:

groups: [
    {
        id: 1,
        items: [
            {id: 1},
            {id: 2},
            {id: 3}
        ]
    },
    {
        id: 3,
        items: [
            {id: 1},
            {id: 3},
            {id: 4}
        ]
    }
]

I have written the following functions to add and remove items:

  removeItem(groupId, itemId) {
    // make a copy
    let {groups} = this.state

    const gIndex= groups.findIndex(g=> g.id == groupId);
    let items = groups[gIndex].items
    groups[gIndex].items = items.filter((i) => i.id !== itemId)

    this.setState({groups})
  }

  addItem(groupId, itemId) {
    // make a copy
    let {groups} = this.state 

    const gIndex = groups.findIndex(g => g.id == groupId);
    let items = groups[gIndex].items
    groups[gIndex].items = items.concat({id: itemId})

    this.setState({groups})
  }

Is there any way to write it cleaner?

\$\endgroup\$

closed as off-topic by Mast, esote, Grajdeanu Alex., 1201ProgramAlarm, yuri Apr 28 at 7:57

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Mast, esote, Grajdeanu Alex., 1201ProgramAlarm, yuri
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ "Note this replicate of my code use 'group' and 'item' that its easier to read than my real code" Yes, in the lines of a Minimum Complete Verifiable Example. Which would've been commendable, if this was Stack Overflow. However, this is Code Review. Here, we require the real deal. The actual code, within it's actual context. Please include your actual code instead of a simplification. We don't deal well with those. Please consider reading our FAQ on how to get the best value out of Code Review when asking questions. \$\endgroup\$ – Mast Apr 18 at 17:29
1
\$\begingroup\$

Not a copy

// make a copy
let {groups} = this.state

You comment that the line following makes a copy. This is not the case, you are just creating a new reference named groups to the array..

Use const

As you do not change the reference you should define groups as a constant.

const {groups} = this.state;
// or
const groups = this.state.groups;

Array.push rather than Array.concat

Array.concat creates a new array, it is more efficient to just Array.push a new item to the existing array

groups[gIndex].items = items.concat({id: itemId});
// can be
groups[gIndex].items.push({id: itemId});

Array.find rather than Array.findIndex

You find the index of the group you want to modify. This complicates the code. If you use Array.find it will return the group and you don't need to index into groups each time.

const gIndex= groups.findIndex(g=> g.id == groupId);
// becomes 
const group = groups.find(group => group.id == groupId);

Common tasks in functions

Between the two functions you repeat some code. I would imaging that other functions also need to access groups by id, make changes, and set the state of the new content. It would be best to provide functions to do that.

getGroupById(id) { return this.state.groups.find(group => group.id === id) },
updateGroups() { this.setState({groups: this.state.groups}) },

Good naming

The functions addItem and removeItem do not indicate that they are related to adding/removing from a group. Better names could be removeItemFromGroup, addItemToGroup

Be consistent

Good code style is consistent. Whether or not you use semicolons you should avoid doing it sometimes, do it always or never. (best option is use them)

Rewrite

Using the above points you could rewrite the code as the following.

Added two functions to do the common task of getting by group id and updating the state with groups.

I assume that the groupId will exist as the code you have given indicates this to be so, if the groupId does not exist your code would throw, and so would the following code.

getGroupById(id) { return this.state.groups.find(g => g.id === id) },
updateGroups() { this.setState({groups: this.state.groups}) },
removeItemFromGroup(groupId, id) {
    const group = this.getGroupById(groupId);
    group.items = group.items.filter(item => item.id !== id);
    this.updateGroups();
},
addItemToGroup(groupId, id) {
    this.getGroupById(groupId).items.push({id});
    this.updateGroups();
},    

If items are unique per group you can avoid the using Array.filter and having to create a new array by splicing the item out of the array using Array.splice

removeItemFromGroup(groupId, id) {
    const items = this.getGroupById(groupId).items;
    const itemIdx = items.findIndex(item => item.id === id);
    itemIdx > -1 && items.splice(itemIdx, 1);
    this.updateGroups();
},

Or if items are not unique, or unique only sometimes you could iterate over the array removing them as you encounter them.

removeItemFromGroup(groupId, id) {
    const items = this.getGroupById(groupId).items;
    var i = items.length;
    while (i--) { items[i].id === id && items.splice(i, 1) }
    this.updateGroups();
},

Safer

The next example guards the function from trying to modify items if the group does not exist.

getGroupById(id) { return this.state.groups.find(g => g.id === id) },
updateGroups() { this.setState({groups: this.state.groups}) },
removeItemFromGroup(groupId, id) {
    const group = this.getGroupById(groupId);
    if (group) {
        group.items = group.items.filter(item => item.id !== id);
        this.updateGroups();
    }
},
addItemToGroup(groupId, id) {
    const group = this.getGroupById(groupId);
    if (group) {
        group.items.push({id});
        this.updateGroups();
    }
},   

And just in case you don't want to duplicate items in a group.

// Only adds items if if it does not already exist in the group.
addItemToGroup(groupId, id) {
    const group = this.getGroupById(groupId);
    if (group && !group.items.some(item => item.id === id)) {
        group.items.push({id});
        this.updateGroups();
    }
}, 
\$\endgroup\$
  • \$\begingroup\$ Fantastic answer, thanks! However, I have just realized this.state.groups should be treated as immutable (reactjs.org/docs/react-component.html#state). That means it should be cloned and then updated using setState() strictly. This is a problem my original code has already... So maybe the final answer could be jsfiddle.net/tnug4v9L/1 ? (I had to remove some of your class methods, because the operations should be done in the clone instead of the class properties) \$\endgroup\$ – Rashomon Apr 19 at 8:48
  • 1
    \$\begingroup\$ @Rashomon Strictly would be enforced (the state data would be frozen and impossible to change) You are safe mutating the state because you set the state at the end of the function with this.setState The state can not be changed while your function is executing so there is no danger of losing state data. All the examples in my answer are safe to use as they are. As for the two extra class functions, that is up to you, but should does not mean must do, JavaScript is fully capable of enforcing such rules, so if you can do, then should do is only advisory. \$\endgroup\$ – Blindman67 Apr 19 at 13:06
  • \$\begingroup\$ Thank you very much for your explanations. Im new to React so I will study carefully the behaviour of the state \$\endgroup\$ – Rashomon Apr 19 at 13:10
1
\$\begingroup\$

For the remove function, how about using a map and filter instead:

removeItem (groupId, itemId){
    let {groups} = this.state;
    groups = groups.map((group) => {
        if(group.id === groupId){ 
           group.item = group.item.filter(item => item.id !== itemId);
        }
        return group;
    });
    this.setState({groups});
}
\$\endgroup\$

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