TL;DR: It might be obvious to you, but not to others.
Why?
More often than not, I used to think that my code is just for me (Narrator: It's not). Whether I'm coding at work or coding for my own side project, at some point someone else will look into the code and try to understand how it works. The last thing you want to happen is them spending too much time doing it.
A little disclaimer: this isn't about rewriting your code and using a totally different programming paradigm (cuz FP the best P), or being overly verbose with variable names and comments. I'll save that for another time. I find that even the smallest changes in the way I write the code could actually help improving the readability. Small enough for a good start.
In the wild
Let's consider a few code examples.
#1
// When the id is in the list, retrieve by id
if (ids.indexOf(id) !== -1) {
retrieveItemById(id);
}
This code works, obviously. I coded it, merged it, went for lunch, and mostly forgot about it. Then someone else reads this code:
If the index of the id in the list of ids does not equal to minus one, retrieve the item by id.
That person mutters, Well, that's one way to put it.
See, this should be avoidable. That person should never have said that. Anyway, let's take a look at how I should fix it.
It is true that .indexOf will return -1 if the value is not in the list, but that isn't what I wanted to say, right? I wanted to do something when the id is in the list of ids, instead of do something when the id is not not in the list of ids.
It is subtle, but the code in my opinion reads better this way:
// When the id is in the list, retrieve by id
if (ids.indexOf(id) >= 0) {
retrieveItemById(id);
}
Heck, I could throw in some ES6 magic and the readability is now times 3000:
// When the id is in the list, retrieve by id
if (ids.includes(id)) {
retrieveItemById(id);
}
But wait, there's more!
#2
// See if any of the results has an error
const isSuccess = results.filter((result) => result.error === true).length === 0;
Now, this is similar to the previous one. If the whole ordeal was whether there are no errors in the results, it should be a statement that returns a boolean. Using .filter then checking the array length makes it longer to read and understand the code.
// See if any of the results has an error
const isSuccess = !results.some((result) => result.error === true)
However, putting a negation in front of the statement makes the code less clearer.
It is a success when not some of the results have an error.
An improved version would be:
// See if any of the results has an error
const isSuccess = results.every((result) => result.error !== true)
The code now reads:
It is a success when every result doesn't have an error.
.some and .every have benefits over .filter because it will stop iterating through the array if the statement is true for the former, and false for the latter. So, even from an efficiency standpoint, .some and .every are better than .filter in this context.
Last one, I promise.
#3
// Only ship item that isn't broken and expired
if (!item.broken && !item.expired) {
shipItem(item);
}
Over time, we noticed that we've been shipping items that aren't in stock. Fine....
// Only ship item that isn't broken, expired, and stock is not zero (duh)
if (!item.broken && !item.expired && item.stock !== 0) {
shipItem(item);
}
I mean, it looks good, right? Right? Wrong.
Again, what I meant isn't exactly ship the item if the stock isn't not there, but ship the item if the stock is there.
It could be that I was following the pattern of the rest of the logic (!item.broken, !item.expired) and subconsciously went for another negation, but for this case, it is better to read if it was done without negation.
// Only ship item that isn't broken, expired, and stock is there
if (!item.broken && !item.expired && item.stock > 0) {
shipItem(item);
}
Conclusion
These examples are really simple, easy-to-avoid issues. And of course, there are other different ways to improve code readability. My main point is that we should try to stop having the mindset of 'my code works and I understand it fine'. Coding isn't essentially just to make it work, it's also to educate others on how (and why) it works.
To me, a good code is similar to how people describe a good UX or a good joke: If you have to explain it, it might not be that good. It's other people that defines whether your code is easy to understand, it shouldn't be you. So, whenever possible, second-guess your code and improve them so that others don't have to.
Of course, this doesn't mean that you would have to go down to a level where a biology major should understand your code. I think what makes a good code is also the balance between making it understandable and having to explain everything. And that takes experience.
Thanks for reading (my first article, btw).
Cover image by Nicolas Thomas on Unsplash.
Top comments (8)
I think sometimes, a change of paradigm can make things more obvious. For example in Clojure
It is quite readable as the vocabulary of functions are more relevant.
Haven't tried Clojure but boy that is a whole new level of readability. Thanks for the tips. I agree, this article is merely baby steps to something bigger like switching to Clojure.
Good tips. I think reading the logic out as a sentence is definitely a great way to realise if it will make sense to others at first glance. If it doesn't read as a proper sentence, then it likely isn't going to easily make sense to others.
Btw, small thing, but
item.stock >= 0should beitem.stock > 0as if the stock = 0 then there is no stock ;)Hey, that is true! My mistake. Thanks for correcting me. I've amended the article.
I didn’t know how much more efficient
.someand.everywere compared to filter. I definitely need to refactor my code that fit this exact use-case. Thank you for sharing!Be careful when making these logic changes. Check that your unittesting will catch incorrect logic since there are several potential logic changes, should && become ¦¦...
Ah yup, that is true. Refactoring should always be done with tests beforehand. I missed that out. Thanks for the tips!
GREAT