3
\$\begingroup\$

Below is a React useEffect hook I used in my application, basically what it does is:

data is an array for objects such as {round:number, date:dateString}.

When the application starts running, data will be an empty array, whenever round gets updated, the useEffect() will be called:

  1. if the data array is empty, we update it with its very first object.
  2. else the data array is not empty and if the last object of data array has its date property equal to props.localeDate, I make a new array with the its last object updated
  3. else the data array is not empty and else the last object's date property is not equal to props.localeDate, I append a new object to data array.

const Chart = ({ round, dateAndTime }) => {      
  const [data, setData] = useState([]);
  const localeDate = dateAndTime.toLocaleDateString();

  useEffect(() => {
    if (data.length === 0) {
      setData([
        ...data,
        {
          round: round,
          date: localeDate,
        },
      ]);
    } else {
      //update item if date matches
      if (data[data.length - 1].date === localeDate) {
        let newData = data.map((item) => {
          let newItem =
            item.date === localeDate ? { ...item, round: round } : item;
          return newItem;
        });
        setData(newData);
      } else {
        setData([...data, { round: round, date: localeDate }]);
      }
    }
  }, [round]);
 return (jsx element);
}

My code works as expected, but I am wondering if there is any way to improve on it? Using if-else looks really ugly.

\$\endgroup\$
1
  • \$\begingroup\$ @BCdotWEB, true, thanks. Updated as per your suggestion \$\endgroup\$
    – Yu Zhang
    Apr 29 '20 at 20:51
1
\$\begingroup\$

Note that you do the same thing if the array is empty or if the last item's localeDate doesn't match - you can combine those two paths.

You only want to update the existing object if the last item exists and its date matches, so I'd first set a flag for that, which can be done concisely via optional chaining.

Then, .map looks a bit verbose when all items of the array are unchanged except for one - you could consider using .slice instead to take all but the last item, spread them into a new array, then add the changed object as the last item.

useEffect(() => {
    // update item if date matches
    const changeExistingItem = data[data.length - 1]?.date === localeDate;
    const updatedData = changeExistingItem
        ? [...data.slice(0, data.length - 1), { ...data[data.length - 1], round }]
        : [...data, { round, date: localeDate }];
    setData(updatedData);
}, [round]);

Also see that you can use shorthand property names to keep things concise: { someProp: someProp simplifies to { someProp in modern Javascript.

As a side note: always use const to declare variables, whenever possible. When you use let, you're warning readers of the code that you may be reassigning that variable name in the future, which results in more cognitive overhead.

\$\endgroup\$
1
  • \$\begingroup\$ thank you so much for your detailed answer. \$\endgroup\$
    – Yu Zhang
    Apr 29 '20 at 11:29

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.