Sorting a list in ascending/descending order with a button toggle

Question

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,
 });
 }
```

• James: Do the contents of the list ever change once loaded?

Answer 1

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 });

• arewa: Only the arrangement changes based on the button toggle ASC/DES.

Answer 2

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.

Answer 3

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.

• Simon Forsberg: 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 :)
• Bojan Bedrač: 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.