Ggthjy

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?

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();

    }

}, 

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});

}