Display toast after action

Question

I have a React functional component where users can add a movie from a list to their dashboard. I'm displaying a toast (Material UI Snackbar) to show the result. The result can be good: movie has been added, or bad: movie is already added to your watchlist.

const addMovie = (movie: IMovie) => {
 const isMovieDuplicate = !checkForDuplicate(movies, movie);
 if (isMovieDuplicate) {
 movies.push(movie);
 const sortedMovieList = sortMovieList(movies);
 setMovies(sortedMovieList);
 addMovieToLocalStorage(sortedMovieList);
 }
 const message = isMovieDuplicate ?
 movie.original_title + ' has been added to your watchlist.' :
 movie.original_title + ' is already added to your watchlist.';
 const variant = isMovieDuplicate ? {variant: 'success'} : {variant: 'warning'};
 displaySnackbar(message, variant);
};
const displaySnackbar = (message: string, type: any) => {
 enqueueSnackbar(message, type);
};
const sortMovieList = (movies: IMovie[]) => {
 return orderBy(movies, [(movie: IMovie) =>
 returnSortType(movie, sortConfig.selectedSortType)], [sortConfig.orderType ? 'asc' : 'desc'],
 );
};

First I call a function that checks the if the current movie already exists in the movieList array. If so it returns a true. The isMovieDuplicate takes the opposite value, so in case of there being a duplicate it will be false so that the if statement is not executed.

In the if statement I push the movie object in the movies array. Then I use a function to return an sorted array. Users can specify how they want to sort the list based on title, release date.

I use the sortedMovieList value to call my React hook setMovies and put he array in the React state. Then I use that same array and put the data in my local storage. At some point this local storage will be replaced with an actual database.

When the if statement is completed I want to show the result in a snackbar (toast). Based on the isMovieDuplicate variable I create the context for the toast and pass that to the displaySnackbar function which renders it on the page.

This all works, but it doesn't feel "clean". There's quite some code, maybe I could move the code for creating the message into the displaySnackbar method?

Answer 1

Not as clean as it could be, some code duplication, and there's also some state mutation in your addMovie function.

Issues

1. movies.push(movie); mutates the current state object.
2. checkForDuplicate isn't a clear name, i.e. checking for duplicates is clear, but what is the return value?
3. Using isMovieDuplicate as the negation of the result from checkForDuplicate is completely counter-intuitive and confusing.
4. When adding data to current state and computing the next state, a functional state update should really be used.
5. Ternary logic could be reduced/simplified.
6. Handle side-effect of persisting to local storage in the component as an "effect" of updating the movies state.

Suggestions

1. Use a functional state update and shallow copy of current state in order to not mutate current state and correctly enqueue state updates.
2. Change checkForDuplicate to checkIsDuplicate to make it more clear the return value is likely a boolean (by isXXX naming convention) and will be true if it is a duplicate.
3. Remove isMovieDuplicate and use checkIsDuplicate directly in conditional test.
4. Remove the ternary. Assume duplicate failure, only update if not a duplicate and adding to movie array.
5. Use an useEffect hook to persist to localStorage (and eventually to DB).

Code

const addMovie = (movie: IMovie) => {
 let message = 'is already added to your watchlist.';
 let variant = 'warning';
 if (!checkIsDuplicate(movies, movie)) {
 setMovies(movies => sortMovieList([...movies, movie]));
 message = 'has been added to your watchlist.';
 variant = 'success';
 }
 displaySnackbar(`${movie.original_title} ${message}`, { variant });
};
useEffect(() => {
 addMovieToLocalStorage(movies);
}, [movies]);

• Peter Boomsma: Thanks again Drew. I love this line setMovies((movies) => sortMovieList([...movies, movie])); very clear and concise. I also like the removal of the ternary statement and make the snackBar output part of the code. Makes it much more readable. 2 questions, if you don't mind: 1. why prefer the useEffect to put the movie in localStorage over the addMovie function. 2. It feels duplicate, placing a movie object in the movies state and also in the local storage. I have 2 places where movies are stored, is there a method where the localStorage acts as the point of truth?
• Drew Reese: @PeterBoomsma Thank you. (1) Separating the state persistence to localStorage is an effect of updating state, i.e. state updated -> persist to longer-term storage. I'm a fan of the Single Responsibility Principle where basically each module/class/function is responsible for a single aspect of the overall functionality of the program. In a bigger system like redux I'd just have a piece of middleware or handler that simply every second or so writes app state to local storage, removing the responsibility from the UI code.
• Drew Reese: @PeterBoomsma (2) Yes, when your application loads (page refresh or native app opens) it can read the persisted app state from local storage and populate the application state. Here your app state is acting as cache.