2
\$\begingroup\$

I have this piece of code written as a function to handle a button-toggle for sorting a list in ascending and descending order. Below is the code which is working as expected, but I would appreciate if someone knows a better, shorter and cleaner way of writing it.

  // method called every time the button is clicked
  // it will change the sorting order 
  //(ASCENDING => DESCENDING => ASCENDING => DESCENDING) in that order

  onSortChange = () => {
      this.setState({
        users: this.state.order
          ? this.state.users.sort((a, b) => {
          if (a.name < b.name) return -1;
          if (a.name > b.name) return 1;
          return 0;
      })
          : this.state.users.reverse((a, b) => {
          if (a.name < b.name) return 1;
          if (a.name > b.name) return -1;
          return 0;
      }),
        order: !this.state.order,
      });
  }
```
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Do the contents of the list ever change once loaded? \$\endgroup\$ – James Feb 2 '20 at 22:21
2
\$\begingroup\$

If the list never changes once loaded, the data would only ever need to be sorted one time (which ever you prefer as the default). Once sorted, you can just reverse the array as you currently are.

Additionally, if your data comes from a database of sorts, you may not even need to sort on the client at all, you could return an already sorted resultset.

So taking all that into account, here's what your code could look like:

// Assuming 'users' is already sorted ASC order, if not then we want to users.sort(...) one time where appropriate
const { order, users } = this.state;
// Copy the array if going in DESC order to avoid resorting for ASC
const sortedUsers = order ? users.slice().reverse() : users;
this.setState({ users: sortedUsers, order: !order });
\$\endgroup\$
1
  • \$\begingroup\$ Only the arrangement changes based on the button toggle ASC/DES. \$\endgroup\$ – arewa Feb 4 '20 at 7:48
2
\$\begingroup\$

Instead of resorting the list in state just store the preferred order in state. Then in render simply sort or reverse the order as needed to present the data. Be sure to use keys that don't change when the order is changed and it'll be quite performant.

\$\endgroup\$
1
\$\begingroup\$

If it works as expected, there's nothing wrong with it. However to improve the readability, I'd separate the setState call from actual sorting. Also be careful when you change the order state variable and when you use that variable to determine the sort order.

orderUsers() {
  if (this.state.order) {
    return this.state.users.sort((a, b) => {
      if (a.name < b.name) return -1;
      if (a.name > b.name) return 1;
      return 0;
    });
  }

  return this.state.users.reverse((a, b) => {
    if (a.name < b.name) return 1;
    if (a.name > b.name) return -1;
    return 0;
  });
}

onSortChange = () => {
  const orderedUsers = this.orderUsers();

  this.setState({
    users: orderedUsers,
    order: !this.state.order,
  });
}

Also as others have suggested, you could just sort it in render(). Keep in mind that you should not mutate state objects. The Array.sort() and Array.reverse() mutate the array in place. You could get unexpected results as a result of this.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Hi and welcome to Code Review. "If it works as expected, there's nothing wrong with it" is not really something I would agree with. Cleaner code is better code :) \$\endgroup\$ – Simon Forsberg Feb 2 '20 at 23:28
  • 1
    \$\begingroup\$ I‘m not advocating unclean code, hence my suggestions to improve it. But one should clean up code when there is good reason to do it, not just for the fun of it, it can be a time sink. \$\endgroup\$ – Bojan Bedrač Feb 3 '20 at 15:19

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.