2
\$\begingroup\$

I am making a Trello clone just for personal project and I wonder if my implementation on mutating the state is good.

My state is basically shaped like this:

boards: [
    {
        id: Number,
        title: String,
        tasks: Array,
        columns: Array,
        columnOrder: Array
    }
]

I want to mutate the state to add the new task to the tasks array given that the payload of the reducer has boardId, columnId, task.

My current implementation of adding a new object to an array:

switch (action.type) {
    case ADD_TASK: {
      console.log(state)
      let newBoard = state.boards.find(
        board => board.id === action.payload.boardId
      )

      let column = newBoard.columns.find(
        column => column.id === action.payload.columnId
      )
      column.tasks = [...column.tasks, action.payload.task]

      newBoard.columns = [...newBoard.columns, column]

      const addToBoard = state.boards.map(board => {
        if (board.id === action.payload.boardId) {
          board = newBoard
        }
        return board
      })

      const newState = {
        boards: addToBoard
      }
      console.log(newState)
      return newState
     //code omitted
 }

Is there a better way to mutate the state rather than my current implementation?

\$\endgroup\$
3
\$\begingroup\$

Mutation is generally discouraged in Redux (and React in general) because it breaks tooling/libraries that expect x === y to mean that x and y are the same throughout time.

If you use the React.useEffect or React.useMemo or React.useCallback dependency arguments, or React.memo, you'll run into problems if you mutate objects that you're passing.

We can clean up your code to avoid this mutation in several passes. First, we'll just remove the logs since they don't affect behavior (and they expose certain implementation details we'll want to change):

switch (action.type) {
  case ADD_TASK: {

    let newBoard = state.boards.find(
      board => board.id === action.payload.boardId
    )

    let column = newBoard.columns.find(
      column => column.id === action.payload.columnId
    )
    column.tasks = [...column.tasks, action.payload.task]

    newBoard.columns = [...newBoard.columns, column]

    const addToBoard = state.boards.map(board => {
      if (board.id === action.payload.boardId) {
        board = newBoard
      }
      return board
    })

    const newState = {
      boards: addToBoard
    }
    return newState
  }
  //code omitted
}

Next, let's inline some of these variables. newState and addToBoard are each only used once:

switch (action.type) {
  case ADD_TASK: {

    let newBoard = state.boards.find(
      board => board.id === action.payload.boardId
    )

    let column = newBoard.columns.find(
      column => column.id === action.payload.columnId
    )
    column.tasks = [...column.tasks, action.payload.task]

    newBoard.columns = [...newBoard.columns, column]

    return {
      boards: state.boards.map(board => {
        if (board.id === action.payload.boardId) {
          board = newBoard
        }
        return board
      })
    }
  }
  //code omitted
}

Next, consider this arrow function:

      board => {
        if (board.id === action.payload.boardId) {
          board = newBoard
        }
        return board
      }

You've made your control-flow more complicated by reassigning the parameter, when an early return would do just fine:

      board => {
        if (board.id === action.payload.boardId) {
          return newBoard
        }
        return board
      }

Also, we only actually need newBoard inside this function (it's never used more than once), so we can move it to where it's needed:

switch (action.type) {
  case ADD_TASK: {
    return {
      boards: state.boards.map(board => {
        if (board.id === action.payload.boardId) {
          let newBoard = state.boards.find(
            _board => _board.id === action.payload.boardId
          )

          let column = newBoard.columns.find(
            column => column.id === action.payload.columnId
          )
          column.tasks = [...column.tasks, action.payload.task]

          newBoard.columns = [...newBoard.columns, column]
          return newBoard
        }
        return board
      })
    }
  }
  //code omitted
}

Now we begin to see some redundancy! For example, newBoard is being initialized to "the board with id equal to action.payload.boardId, but we already have a name for it: that's just board! So we can actually get rid of the name newBoard entirely:

switch (action.type) {
  case ADD_TASK: {
    return {
      boards: state.boards.map(board => {
        if (board.id === action.payload.boardId) {
          let column = board.columns.find(
            column => column.id === action.payload.columnId
          )
          column.tasks = [...column.tasks, action.payload.task]

          board.columns = [...board.columns, column]
          return board
        }
        return board
      })
    }
  }
  //code omitted
}

But now we need to deal with the unnecessary mutations. We don't want to modify board (or any of its fields, like columns). Instead, we want to return a copy with changes.

We can do this in parts. For example, instead of

          board.columns = [...board.columns, column]
          return board

we can instead say

          return { ...board, columns: [...board.columns, column] }

which copies board and then replaces the columns field with the new given value.

We now have

          let column = board.columns.find(
            column => column.id === action.payload.columnId
          )
          column.tasks = [...column.tasks, action.payload.task]
          return { ...board, columns: [...board.columns, column] }

and similarly, we can change the last two lines to obtain the non-modifying equivalent:

          let column = board.columns.find(
            column => column.id === action.payload.columnId
          )
          return {
            ...board,
            columns: [
              ...board.columns, { ...column, tasks: [...column.tasks, action.payload.task] }
            ]
          }

However, at this point, it seems likely that you have a bug here. This change duplicates the entire column, in addition to adding the single task to it! It seems that you instead wanted to just add a task to the one column with matching ID, which we can do instead:

switch (action.type) {
  case ADD_TASK: {
    return {
      boards: state.boards.map(board => {
        if (board.id === action.payload.boardId) {
          return {
            ...board,
            columns: board.columns.map(column => {
              if (column.id === action.payload.columnId) {
                return { ...column, tasks: [...column.tasks, action.payload.task] };
              }
              return column;
            })
          }
        }
        return board
      })
    }
  }
  //code omitted
}

This gives us a fully cleaned-up and immutable version of your code. Of course, you might rightfully observe that this is more-heavily nested than your original, which is a common result of switching to non-mutating state. You can use some existing ones or build your own.

For example, in this case, you're frequently using the following pattern:

  • find a matching object in an array
  • return the array with all other objects unchanged, and apply a transformation to the one that matches

This is easy to write as a helper function. Here mapIf is used to select the right board and then the right column:

function mapIf(array, condition, transform) {
  return array.map(item => condition(item) ? transform(item) : item);
}

switch (action.type) {
  case ADD_TASK: {
    return {
      boards: mapIf(state.boards, board => board.id === action.payload.boardId, board => ({
        ...board,
        columns: mapIf(board.columns, column => column.columnId === action.payload.columnId, column =>
          ({ ...column, tasks: [...column.tasks, action.payload.task] })
        ),
      })),
    }
  }
  //code omitted
}

Even this is still a bit long, so you can write more helpers, like a hasId higher-order function:

function hasId(id) {
    return item => item.id === id
}

hasId returns a function that checks whether its argument has the original id. So we can instead write:


function mapIf(array, condition, transform) {
  return array.map(item => condition(item) ? transform(item) : item);
}

function hasId(id) {
  return item => item.id === id
}

switch (action.type) {
  case ADD_TASK: {
    return {
      boards: mapIf(state.boards, hasId(action.payload.boardId), board => ({
        ...board,
        columns: mapIf(board.columns, hasId(action.payload.columnId), column =>
          ({ ...column, tasks: [...column.tasks, action.payload.task] })
        ),
      })),
    }
  }
  //code omitted
}

Lastly, you frequently only want to touch one (or a couple) fields in an object. Here's one helper you can write to clean up access to the tasks and columns fields:

function onField(field, func) {
  return item => ({ ...item, [field]: func(item[field]) })
}

this helper allows you to write (for example) onField("x", val => val + 1) to get a function that increases the .x field of its argument by 1, leaving all the others alone. You can use it like so:

function onField(field, func) {
  return item => ({ ...item, [field]: func(item[field]) })
}

function mapIf(array, condition, transform) {
  return array.map(item => condition(item) ? transform(item) : item);
}

function hasId(id) {
  return item => item.id === id
}

switch (action.type) {
  case ADD_TASK: {
    return {
      boards: mapIf(
        state.boards,
        hasId(action.payload.boardId),
        onField('columns', columns => mapIf(
          columns,
          hasId(action.payload.columnId),
          onField('tasks', tasks => [...column.tasks, action.payload.task]),
        )),
      ),
    }
  }
  //code omitted
}

This change suggests a new way to write mapIf, so that it works better with onField, by making it return a transforming function instead of directly taking the array as an argument:

function onField(field, func) {
  return item => ({ ...item, [field]: func(item[field]) })
}

function mapperIf(condition, transform) {
  return array => array.map(item => condition(item) ? transform(item) : item);
}

function hasId(id) {
  return item => item.id === id
}

switch (action.type) {
  case ADD_TASK: {
    return onField(
      'boards',
      mapperIf(
        hasId(action.payload.boardId),
        onField(
          'columns',
          mapperIf(
            hasId(action.payload.columnId),
            onField('tasks', tasks => [...column.tasks, action.payload.task]),
          )
        ),
      ),
    )(state);
  }
  //code omitted
}

The main downside to this approach is that there's lots of nesting that can make it hard to understand what's going on. So if we make one more transformation, and write a chain helper, we can get the (in my opinion, rather beautiful):

function onField(field) {
  return func => item => ({ ...item, [field]: func(item[field]) })
}

function mapperIf(condition) {
  return transform => array => array.map(item => condition(item) ? transform(item) : item);
}

function hasId(id) {
  return item => item.id === id
}

function apply(transform) {
  return func => item => transform(func(item));
}

function chain(func, ...funcs) {
  if (func === undefined && funcs.length === 0) {
    return item => item;
  }
  return func(chain(...funcs));
}

switch (action.type) {
  case ADD_TASK: {
    return chain(
      onField("boards"),
      mapperIf(hasId(action.payload.boardId)),
      onField("columns"),
      mapperIf(hasId(action.payload.columnId)),
      onField("tasks"),
      apply(tasks => [...tasks, action.payload.task]),
    )(state)
  }
  //code omitted
}
```
\$\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.