Guidelines for writing readable code
alemil.com
alemil.com
I can see what code does, it written there, in code! I want to know what its purpose is so I can decide if I have to remove/refactor/fix it.
Often code doesn't do what was intended (a bug) and by only commenting what it does the bug transpires into the documentation as well.
Compare "// loop over array of numbers and add them together" with "// compute sum of all product prices in cart".
The first is only helpful if you are explaining a language to someone or are not confident about the language. The second tells the importance of this piece of code in the greater whole.
Compare:
findNeedles(haystack, needlesList)
[insert a few nested loops which are hard to follow unless you run the program to see what it does]
vs:
countWordsInString(str, wordsList)
[same code as above]
There is sometimes a great deal that can usefully said, about what a function is for, that is only apparent in the context of its use. If the purpose of a() is accomplished by calling a1(), a2() and a3(), it would not be unusual for the purpose of a2(), for example, to only be understandable in the way a1(), a2() and a3() work together to satisfy the purpose of a(). If the only words you are allowed to write are the names of the functions, there is nowhere to document this interaction (especially if the name of a() is supposed to say what it does, not how it works.)
The more you break down a solution into functions, the more the satisfaction of the purpose of the program becomes a consequence of how the functions interact.
For an example, look at the comments to the function checkListProperties in christophilus' SQLite example [1], and consider how you would convey the same information through function naming. Then look me in the eye and tell me that's the way you write code.
Reading through code that looks like this[0] is so much nicer than reading through code that looks like this[1].
[0] https://www.sqlite.org/src/artifact/9711a7575036f0d3
[1] https://github.com/mysql/mysql-server/blob/b93c1661d689c8b7d...
I would suggest to use commenting as soon as you're doing something that's not immediately obvious to the reader, which can be all of the time or almost never depending on what you're doing. If would like to see some examples of Rails or JS code that needs comments according to you to have a better discussion about it.
I find both of your examples readable to about the same level; MySQL one because the naming is helpful (`number_of_bytes_read_from_buffer`) and SQLite one because of the comments - I would be lost without them. For example, why is a function that does "Release the STATIC_MASTER mutex" called "leaveMutex"? On the other hand, SQLite comments explain much more about how the code is used, what assumptions are made, so this is really a bonus.
As I've matured as a programmer, I've found that commenting everything is actually an anti-pattern. Every line of code is a liability that contributes to maintenance costs, even comments. Just as you should think about every executable line in your source code, you should put the same thought into all non-executable lines. I prefer to comment judiciously and try to write my thought process and anything that's likely to be confusing to help future developers understand the overall approach and intent of the code without trying to lay out the intent of every single line. Lines that are particularly complex get a comment, but anything simple stays uncommented, especially if it's idiomatic for the language.
If, to take your example, the code were:
let cartTotal = products.reduce(0, (total, product) => { total + product.price });
// cartTotal is computed in cents to avoid floating point math/rounding
// and must be divided by 100 to get the dollar amount.
I've also realized that, by far, the best comments I can leave are unit tests. They never become incorrect because they're forced to update with the code. When I'm trying to learn a new code base, I always start by running the tests and, if they pass, reading the tests before I read the code they test. There is no better way of understanding what use cases a developer thought were most important than the automated tests s/he felt were necessary. I love that new languages like Rust give developers the ability to combine documentation and tests and I hope many more languages follow that example.
The second best documentation are function/method/class/object/struct names. It's very important to get them right because they not only provide meaningful information in the place they're defined, they also provide useful information wherever they're used. Try to avoid the following:
/**
* Helpful comment
*/
class UnhelpfulName
...
What's not useless is a comment like "// Ignoring tax rates for sum because some products are taxes differently" - Comment the why, the business decisions, rules, technical debt, etc that's not immediately relevant when looking at that one method. Anything else is clutter.
The main point I'm getting at is the intention. All coding is is a translation of your intention to something machine executable. And often mistakes are made in that. That's why I comment to keep the intention close to the implementation.
As for number 9, I'll agree with this, so long as it's in the standard libraries (which are slow to change and unlikely to break/change functionality when they do change). But when you use 3rd party libraries, you are still responsible for that code in the long run. You'll have to ensure it remains up-to-date, ensure that you're testing that it's doing what you intend for it to do, and when it's eventually compromised or removed, you'll have to deal with that too.
See: Left Pad.
Sure, but you very rarely have to do it alone, and you'll only have to do it for a very small fraction of libraries.
A similar issue I've seen a few times is people implementing an API and the use of that API in two separate changes, and as a result creating an over-generalized and harder-to-test API that tries to anticipate lots of use-cases, instead of a much simpler API that only exposes/tests what's actually needed.
Known as "Three Strikes and You Refactor"[0]:
- The first time you do something, you just do it.
- The second time you do something similar, you wince at the duplication, but you do the duplicate thing anyway.
- The third time you do something similar, you refactor.
Sadly the real world isn't so accommodating.
Alternatively, if you really want to be exacting about this, there are code analysis tools that will do it for you.
Like walking through a doorway, descending into a function can make you lose the context surrounding that function, making it difficult to see what is actually common between the 2, 3, or 4 different invocations of that seemingly common code.
We instinctually abstract by fitting n use cases into 1 abstraction, when in fact we should be inverting the dependency graph and writing or reusing n abstractions for each use case.
Too much copy-pasta is a great reason to refactor.
It's not enough for the three passages to happen to be identical at this moment in time. You've also go to be sure that, going forward, they will need to evolve identically and in lockstep.
> Code duplication is far cheaper than the wrong abstraction.
3. Simplicity is king.
5. Naming is hard, but it's important.
9. Prefer internal functions over custom solutions.
11. Avoid creating multiple blocks of code nested in one another.
14. Split your classes to data holders and data manipulators.
To be honest all of them really feed into number 3. The lower the cognitive burden to reading your code the easier it will be the maintain and work with.
Sure, at any given point the stack is shorter than the list so there's less things to keep in mind, but for me dealing with mental lists is much easier than dealing with mental stacks.
In the particular case of mixing data storage and data manipulation in a single class: It tends to yield classes that continuously accumulate new functionality over time. They also tend to be difficult to refactor int a set of smaller classes. The level of impact on consuming code is very high, because splitting those classes tends to also force major changes to anything that interacts closely with them. And, since all that functionality lived inside a single class, it's likely to be internally resistant to change, too. It's really easy to treat "encapsulation" as a reason to not worry overmuch about shared use of glo^H^H^H instance variables.
Does this kind of stuff mean that OOP didn't turn out to be all it was cracked up to be? Yup. Does this mean we're throwing the whole idea out? Nah, it's just evolving. The original idea's been augmented with other useful ideas like the interface segregation principle, the Liskov substitution principle (mixing behavior with data makes obeying LSP very difficult), and acknowledging the value of referential transparency.
Because it still allows you to write to a higher level of abstraction. So you can have all sorts of different data storage classes - lists, sets, hash maps, etc. - and decide that they all implement an "enumerable" interface, and then any functions that just need to be able to apply some operation to every element in a collection can work with every single one of your collection types.
Haskell has ways of getting us that sort of thing with only structs and bare functions - and does a really nice job of it, too - but most of us aren't working in Haskell.
If you can get over of him calling everything "pure evil", the simplicity he tries to achieve with his controversial view on what is true OOP is quite interesting.
I think that instead of this splitting to data and functions a better solution is to use Single Responsibility, Tell don't Ask, and Demeter's law. This way one can make classes small enough so functionality accumulation won't be a problem and a class will tell what to do only to their "neighbouring" classes. This way every change in the code will be somewhat local. Of course this is an idealistic view which fades with every ORM entity read... :)
Classes can still be parameterized in a way that modules in procedural languages can't, which opens up all sorts of flexibility that wasn't available, or at least was exceedingly awkward to accomplish, in non-OO procedural languages. For example, you're going to want some sort of post-procedural language if you want to use dependency inversion.
An example of where rules of thumb about abstractions turn into poorly designed code is when processing large arrays or vectors of data. It is common to model your data in some kind of product type like a struct or union and process each item using some method of iteration. However if the main usage pattern of our data is to perform this transformation over the vector within an outer loop we've shot ourselves in the foot from the get-go. Our caches are constantly swapping data. This is called, premature pessimization.
So don't get trapped into thinking that our job is only about learning SOLID principles or various aphorisms and mnemonics. The other side of our job is transforming and moving data.
* What it does: The code answers this in a clear manner
* Who: did it, the code repository answers this.
* Where: is used, your IDE should be able to show this
* When: was it made code repository, when is used stack traces
* Why: why does it exist? that is the only part that needs to be in a comment.
Brainstorm, outline, draft, revise, rearrange. Have a high-level structure that makes sense, clean up “sentence structure” and “word choice”. Often the best path to “readable code” is “brain dump a mess, than revise” rather than “write a perfect first draft”.
Returning in the start is as anti-pattern for me as goto in the middle. both of them leave you with lines of code that only a debug session (in mind or in action) can tell you what lines will be called. I will choose nested code any-day if that means i can simply follow what `if`s are relevant rather than guessing.
1. You are guarding for errors or invalid states - you should use early returns or thrown exceptions because these are unexpected states.
2. Your business logic has to branch - you should use a nesting if (or various other options that aren't in scope here, like double dispatch, pattern matching or polymorphism).
Anyhow, with both approaches, nested or early return, the reader has to read the function from the beginning (or read back until the relevant part) to grasp when exactly a code portion will be executed. In both cases, all if's are relevant, there's no guessing.
if (product.hasPrice){ if (someOtherCondition){ Cart cart = cartService.getCartForCurrentUser(); cartService.addToCart(product,1); } }
to
if(canSellProduct(product)){ addToCurrentCart(product) }
I don’t get it. How do early returns not convey assumptions that can be made in the rest of the function?
> Returning in the start is as anti-pattern for me as goto in the middle. both of them leave you with lines of code that only a debug session (in mind or in action) can tell you what lines will be called.
This is really confusing to me. What do you mean by “in mind or in action”?
In any case, if you have runtime conditions you can’t tell what code will be executed in general just by looking at the code regardless of whether you’re looking at nested code or code with early returns.
> I will choose nested code any-day if that means i can simply follow what `if`s are relevant rather than guessing.
Are the conditions for early returns not relevant or something? What about them leaves you guessing?
Do you mind expanding a bit more on this? From what I understand, if you have early returns the only way you can get into the rest of the function is if all of the conditions pass, not any arbitrary combination of them.
Also, do you have an example of a more complex function with early functions that you find difficult to read?
That way there is no need to scroll up for preconditions, be it a nested tree or some returns at the start.
And when you move a small piece of code, you're told exactly what you should be thinking about, you need to decide what to do with that assert that represents an assumption.
If I'm extracting code to be available for more general use, I usually prefer to try to forget the preconditions of the source function and apply just the preconditions that are applicable for the now-standalone code. In that case, once I'm done with the extraction I'd need to ensure that the preconditions of the source function match the preconditions of the extracted code, and nesting doesn't really affect that; if anything, having the preconditions all up front would make things a bit easier by having everything in one place.
As for moving blocks of code around, I can't say I've had much need to move code across "precondition domains", for lack of a better term. Perhaps I just haven't run across the right situation yet.
But the article is a good approach to thinking about a multitude of issues. Even if we nitpick or disagree with specific points, the exercise of thinking through it all will certainly result in improved code.
I honestly cannot think of a reasonable argument for why constant values should not be in static constants or enums. For one, why would you want to re-type the value over and over? And two, it only takes one typo in one manually typed value to introduce a frustrating bug! It's such an easy bug to avoid and costs almost zero to do correctly! Honestly, devs manually typing values that are effectively consts is one of my biggest pet peeves, you're just creating easily avoidable problems for yourself and your team :).
My personal opinion is that values should be in constant values if:
- Its repeated more than once or referenced outside of the file
- The constant value is a "magic number" where the meaning of the value isn't obvious to outsider
If the value is used only once and the meaning of the value is clear, I think extracting a constant out is needless indirection.
For example:
if (flags & 45676 == 0) {}
if (flags & ROLE_ADMIN == 0) {}
pool.setDefaultTimeoutMillis(1000);
This number is why nesting code is almost always heinous. Once the number of nested conditionals exceeds working memory, your monkey brain cannot hold the state and drops all the variables.
Deeply nested statements is my number one code smell, and can almost always be easily refactored.
[0] https://en.wikipedia.org/wiki/The_Magical_Number_Seven,_Plus...
Sometimes it is more readable to duplicate code or to have functions that do more than one thing. If I have a function and I need to follow four or five levels of function calls and piece things together to understand what that function does, then I can’t read the code...
> 5. Naming is hard, but it's important.
In other words, you should be able to understand what a function does just by the naming of the function itself. When you have a function that "calls 5 other functions" you should be able to understand what it does by just looking at the 5 function calls, without needing to go inside those child functions to check it out.
The only case where this doesn't apply is when you are tracking a bug and the input/output of the function doesn't make sense. That's the only case I can think of to actually dig deeper...
24: Avoid the 'inner-platform effect' [0]
Make proper use of the machinery of the standard library.
This makes your code more readable, more portable, shorter, faster, and less error-prone, and it doesn't even take any real effort - indeed, it makes writing things easier!
The standard-library was probably written by someone smarter than you, almost certainly someone more familiar with the language than you, and it's definitely better tested, better documented, and, importantly, better known, than whatever you were about to write. This is a pretty basic point, perhaps, but most of them are.
Special case: use standard-library data-structures. If you're implementing your own hashset over a raw array, rather than using the standard HashSet class/template, you'd better have a very good reason.
I don't like the look of #10. I'm a believer in single-point-of-return.
Yes, this is true.
But for me, people get it the wrong all the time.
If you have an object which you have to check yeah don't check for everything you could ever imagine. Like I have seen code where the actual check was longer than the code after.
But if you are looping through a big array and I mean big not your little 1000 index array and you think any loop is good enough and suddenly you have a reactive system which was the simplest way to code but the thing is over reactive so your calling your function with a slow loop function all the time just because was simpler to read or to implement your just doing more harm with that rule than actual benefit.
This is also the reason why people use `.map()` in javascript to loop through stuff without ever knowing what map is actually for.
(Yeah I know this is not good HN ettiquete, but I just could not resist in this case. Sorry.)
That was the irony :)
https://www.youtube.com/watch?v=rFejpH_tAHM
https://talks.golang.org/2015/simplicity-is-complicated.slid...
But people use it for just looping through an array without creating a new one
It is our responsibility to look at what is needed and what complexities, if any, are required to achieve the results. Of course, the opportunities to actually talk with the end-user of any software system is rarely available in the normal course of development, especially for junior programmers. It would be a useful tactic to have all programmers have to deal with the end-users and actually face their complaints and have to learn what the code is doing to those end-users.
If you’re going to parse a CSV file, please, just use a standard library for your language - please don’t just split on commas and call it done. The only case where it might be acceptable to do it yourself is if you have complete control of the input - but that would to me imply a static data that could be added to the code base instead of a CSV file.
One of my worst professional programming experiences was having a senior developer who took apart usage of map, filter, reduce, and replaced them with if else loops, and sometimes breaking things in the process.
The idea that you would one day have a junior who would fail to understand a relatively simple concept seems like fantasy. Let them get stumped once, and then teach!
Like the article said, something like this is better than an if else loop: $variable == $x ? $y : $z;
But if you write stuff like this:
$variable == $x ? ($x == $y ? array_merge($x, $y, $z) : $x) : $y;
I am going to be annoyed. In the second case, please use if else.
The primary reason to avoid code duplication is to improve maintainability and refactorability, not to improve readability.
The clearest code that I have read used consistently single-letter variable names. If a variable name holds a meaning, you are then repeating this information each time you use the variable. It is better to name the variable "a", and put a comment upon its declaration. This is the convention used in mathematics and physics, and it works really well.
Go sometimes gets a lot of flak for its style here (partially because it's sometimes misapplied) but I think it works pretty well: it's fine to use a short non-descript variable name like `a` if its uses only span a few (~5-10) lines (or other certain special cases like `i`, `j` for loop indexes), otherwise use longer descriptive names.
If you create a meaningful name, then that means I can understand the code more quickly with less effort, and save that mental effort for more productive tasks.
If you're programming professionally, why would you prefer greater elegance and conciseness... over less work? It feels like a harmful over-optimization.
Have you ever read an applied math book? They do not seem to have this problem.
My point is that
int d; // dimension_of_the_space
int dimension_of_the_space;
Working through an applied math book takes an entire semester with many, many hours of study, and you're expected to master the material inside out.
Programmers generally do not have anything like the luxury of that kind of time, nor are they expected to reach that level of mastery with the codebase -- they need to quickly identify where changes need to be made, make them, ensure test coverage continues to be complete and passes, and move on to the next thing.