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\$ Commented Feb 2, 2020 at 22:21

3 Answers 3

Reset to default
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\$ Commented Feb 4, 2020 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\$
0
\$\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\$ Commented Feb 2, 2020 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\$ Commented Feb 3, 2020 at 15:19

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.