Guidelines for writing readable code

https://alemil.com/guidelines-for-writing-readable-code

by ristem • 8 years ago

135 98 8 years ago

yonixw4• 8 years ago

I really don't agree with no 11. My reasoning is that the reader should know at each line what conditions are assumed.

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.

beaconstudios• 8 years ago

I think there are two situations where the return early and nesting if options come into play, and each situation warrants a different strategy:

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).

erAck• 8 years ago

Try to "simply follow" when it's nested twelve levels deep.

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.

yonixw4• 8 years ago

when you want to change someone else code, nested blocks lets you know what conditions are needed for each line. when they are gathered at the start you know the condition for the function but for each line you could start a discussion what conditions are necessary and thus make the code atomic (change all by purpose\tests or change none)

aw16211074• 8 years ago

My reasoning is that the reader should know at each line what conditions are assumed.

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?

duncan-donuts• 8 years ago

In mind or in action means either your running the logic through your head to debug or you’ve set a break point and you’re actively debugging

yonixw4• 8 years ago

they are all gathered at once.the following lines could assume any combinations of those conditions. in simple examples it is not an issue but as the function get more complex I found it excruciating.

dsego2• 8 years ago

I think he means that when you see a block of straight non nested code, you assume it can be yanked out or moved around without penalty. That is, the conditions and the actual code are not visibly connected and you have to trace the dots. But when it's nested it's immediately more clear that the nested code depends upon some conditions.

codingdave2• 8 years ago

And I don't agree with #8, because roles can always be abstracted into, for example, an isAdmin() and then none of the concerns apply.

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.

lsadam02• 8 years ago

It sounds like you really just disagree with the example given, not necessarily the rule itself. You're arguing that `isAdmin()` might require a different implementation in a difference scenario, but you have not made a case that the string "admin" should not be constant or enum.

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 :).

hnzix• 8 years ago

Let me introduce you to The Magic Number Seven, Plus or Minus Two [0]. This is the amount of state your monkey brain can hold in working memory.

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...

bognition• 8 years ago

Very good stuff here. I totally agree with many of these points. Especially, these 5.

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.

yonixw4• 8 years ago

I disagree with no.11 ... I prefer nested code of if's because as a reader I learn which conditions are relevant and assumed, rather than check-it-all at the start.

aw16211074• 8 years ago

How does checking at the start not convey what conditions are relevant and what assumptions can be made in the rest of the function?

wongarsu• 8 years ago

If I put all my early exits at the start, for the remainder of the function I only have to remember the list of preconditions established by the early exits. With deeply nested code I instead have to keep a stack of currently fulfilled conditions in mind.

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.

ThorinJacobs• 8 years ago

What level of nesting are you referring to here? I ask because I experience the opposite - with more than one or two levels of nesting I have difficulty keeping track of the condition the block expects. Depending on the size of the method sometimes even one level can make me stumble a little.

de_watcher2• 8 years ago

Number 14 is funny: it's like "do classes, but don't do what classes actually do". The first paragraph then goes about setters and getters that are specifics of some particular styles in some particular languages (and why you even need setters if you want to be clear about "not manipulating the data"?).

bunderbunder5• 8 years ago

#14 fits into a fairly well-established trend of thought on object-oriented programming. A lot of us have noticed, over the 20-ish years since OOP really got popular, that doing it in the way most of us were taught in CS 101 (or wherever) tends to produce code that is difficult to maintain over time.

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.

aequitas4• 8 years ago

My one addition to this would be: comment what you intent the code to accomplish not what it actually does.

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.

v3gas• 8 years ago

And the second should, imo, not be a comment, but rather a function name.

nickjj• 8 years ago

The worst is when that second comment isn't there and you're left wondering what's going on, which leads back to naming things well.

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]

aequitas4• 8 years ago

The problem with function names is that there are other factors that decide them. Conciseness of code being one. And as I commented below. At the time you write code it might not be obvious it will turn into a function. With a proper comment that decision can more easy be made later on.

mannykannot• 8 years ago

That only works in simple cases. Most cases are simple, but the ones that matter most are the ones that are not.

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.

[1] https://www.sqlite.org/src/artifact/9711a7575036f0d3

dsego2• 8 years ago

And then often that comment means that you can extract the code it describe into a separate function of the same name, e.g. price_sum(products).

aequitas4• 8 years ago

That is the choice you often make later on. But might not always be obvious at that time, otherwise leading to overoptimizing everything in functions.

christophilus• 8 years ago

After 20 years as a software engineer, I've started commenting heavily. I used to comment sparingly. What made me change was a combination of reading the SQLite and Redis codebases and inheriting two biggish projects (one Rails, and one JS) which had no comments.

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...

dep_b• 8 years ago

The problem is there's not just one type of code. Shared low-level code needs much more comments as shuffling a bunch of words through memory often is a bit abstract and you don't get the "why" by just looking at it, while writing business code that basically glues the UI to the DB using a lot of frameworks for the ugly parts can (and should) read like English.

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.

anyzen• 8 years ago

I made an opposite journey. While working in C and Java I commented heavily, but in Python I prefer to just leave a comment here and there if I'm not sure the reason for the code and its intent are clear - but at the same time a comment is also a powerful signal for me to rethink that piece of code and make it more obvious.

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.

curun1r• 8 years ago

Even your second version of the comment is too what and not enough why. What comments are worse than no comments. They take up valuable space and become a liability if they're not kept in lock-step with the actual implementation.

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

It probably does not need the comment you've listed. The names alone convey intent. If, however, you're were doing something like integer math, you would put a comment to the effect of

    // cartTotal is computed in cents to avoid floating point math/rounding
    // and must be divided by 100 to get the dollar amount.

That conveys information that is very difficult to extract from the code or the variable names.

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
    ...

The reason this is such a big problem is that it requires the developer or the IDE to find the comment whenever the name is referenced elsewhere in the source code. Names permeate throughout the codebase much better than comments. And if you lean too heavily on comments to provide meaning that the names do not, you end up having to write the same comments in many locations.

pc862• 8 years ago

Comments should never tell you what the code does, they should tell you why it does it. If you have a method named "SumCart" and it's clear that it loops through the products in the cart and returns the sum of the prices, a comment "// compute sum of all product prices in cart" is absolutely useless. All it does is take up space.

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.

aequitas4• 8 years ago

The comment example tells what I intend the code to do, in this case provide the sum. Not how it does it. Maybe my example was to trivial to justify a comment I agree with that, a proper function name would have sufficed.

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.

oldandtired• 8 years ago

Number 3 has some merit. But. Number 3 is also the cause of much grief. I have had to deal with the consequences of "gun/guru" developers doing 3 because it is easy, without them considering the effects on those who use the systems in question.

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.

d--b• 8 years ago

It’s generally good advice. But some items do not acknowledge that there often if a trade-off to be made and the trade-off depends on the program you make.

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...

kilburn3• 8 years ago

You can definitely go silly by splitting your functions too much. However, most times I hear this complaint, the real reason is:

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...

kelnage• 8 years ago

Urgh, I was nodding away until I reached guide 16. Although the idea it expresses is fine, their example of parsing CSV files yourself is a terrible one, especially since they link to a previous article where they anticipate some issues that can be encountered when parsing CSVs, but ignore the legion of others (different separators, quotation characters etc).

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.

lampe35• 8 years ago

`3. Simplicity is king.`

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.

kilburn3• 8 years ago

Oh, the irony.

(Yeah I know this is not good HN ettiquete, but I just could not resist in this case. Sorry.)

lampe35• 8 years ago

????

lloeki• 8 years ago

May I recommend watching "Simplicity is complicated" talk by Rob Pike:

https://www.youtube.com/watch?v=rFejpH_tAHM

https://talks.golang.org/2015/simplicity-is-complicated.slid...

lampe35• 8 years ago

Thanks, I will.

sethammons2• 8 years ago

How is .map() not a slow loop? I'm not a js Dev, but my understanding is it loops over all elements in a list, applying a function and creating a new list. With a loop. That would be as fast as any other loop. What am I missing?

lampe35• 8 years ago

The point is that map itself returns an new array.

But people use it for just looping through an array without creating a new one

jowiar• 8 years ago

It’s a bit more meta than what this guide covers, but I wish something was said about treating writing code as fundamentally a writing process. Most documents like this focus on “what readable code is”, and not “how do I achieve that product”.

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”.

elviejo• 8 years ago

My personal documentation guideline is, just answer the five W questions:

* 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.

slededit• 8 years ago

I like to know how well the author understood the problem. Unfortunately git allows people to rewrite history and hide the guess+check process many go through.

dorfsmay• 8 years ago

I'd add, beside obvious docstrings, comment to explain why the code does something when it's not obvious (say a corner case).

enriquto5• 8 years ago

Disagree strongly with the comment about naming : "Names of variables and functions should be distinct and provide a general idea of what they do".

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.

ab71e5• 8 years ago

Maybe some of them, like in tight loops. But the most difficult code to understand for me is Matlab scripts written by physicists, naming variables a,b,c or i,ii,iii etc. Variables with longer lifespan should have more explicit names in my opinion.

enriquto5• 8 years ago

Of course! I am talking about the names of local variables, not the names of externally visible functions or global variables.

crazygringo2• 8 years ago

It's the most elegant code, sure. But it also creates more work for me to read it, because I need to build up a mental map memorizing {letter => meaning, letter => meaning, ...}.

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.

enriquto5• 8 years ago

You do not need to memorize anything if your function fits in a single screen and you have less than 10 variables at the same time (which are good conventions as well).

MaxBarraclough2• 8 years ago

I'd make the following addition, which I was surprised not to see:

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.

[0] https://en.wikipedia.org/wiki/Inner-platform_effect

kaltherX• 8 years ago

Isn't this #9 Prefer internal functions over custom solutions?

MaxBarraclough2• 8 years ago

Oops, I missed that! Yes, it's pretty close.

I don't like the look of #10. I'm a believer in single-point-of-return.

falcolas3• 8 years ago

Number 6 (DRY), and number 9 (use libraries), both lead to problem 16 (leaking abstractions). Personally, I follow a rule I read in a blog some time ago and don't try to de-duplicate code until I've copy and pasted it three times. That way I know what the actual pattern that's being repeated is, not just what I think the pattern is.

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.

bryanlarsen• 8 years ago

"But when you use 3rd party libraries, you are still responsible for that code in the long run."

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.

alecbenzer2• 8 years ago

+1 on "premature generalization"

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.

kevinconaway2• 8 years ago

Personally, I follow a rule I read in a blog some time ago and don't try to de-duplicate code until I've copy and pasted it three times

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.

[0] http://wiki.c2.com/?ThreeStrikesAndYouRefactor

cimmanom• 8 years ago

The problem with this is the cases when you don’t remember that you (or someone else) already used this same snippet twice. In large codebases with a lot of people working on them, this can expand to dozens of copies.

bunderbunder5• 8 years ago

I wouldn't automatically refactor on the 3rd time.

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.

lojack• 8 years ago

Sandi Metz did a great talk on this that can be summed up in one really good quote

Code duplication is far cheaper than the wrong abstraction.

https://youtu.be/8bZh5LMaSmE

hikarudo• 8 years ago

I highly recommend "The Art of Readable Code: Simple and Practical Techniques for Writing Better Code", by Dustin Boswell and Trevor Foucher. It's an easy read, and well explained with good examples.

Sir_Cmpwn• 8 years ago

I want to expand on #2 - the right tool for the job might not be a flashy new library/framework/tool, but it might be one you don't already know. You shouldn't pick new technology because it's new, but you should have a surface-level understanding of what lots of technology does so you might be able to connect the dots with your use-case and explore something foreign in depth. Equally risky to using flashy tools is over-reliance on the old tools you know, which may not be best suited to the task.

agentultra• 8 years ago

I also suggest consider your data. There are two main problems we face as programmers: managing complexity and transforming data. The former gets the majority of the treatment in articles like this but I believe the latter is often more important. We must consider the shape of our data, the usage patterns of our data, and consider structuring our programs to accommodate such patterns and transformations.

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.

ummonk• 8 years ago

“Do not duplicate your code” rarely improves readability, unless the part that you dedupe is a simple semantic component of its own and not closely intertwined with the nonduplicated code it is embedded in. More often, it can lead to the hidden trap of abstractions mentioned in the OP’a post.

The primary reason to avoid code duplication is to improve maintainability and refactorability, not to improve readability.

esdkl222• 8 years ago

I have a question about point #12. Are most people in agreement that the if else loop is preferrable over a ternary operator?

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!

PunchTornado• 8 years ago

no, but it all comes to the complexity of the operation.

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.

esdkl222• 8 years ago

You're correct. I didn't look closely enough at the finalized piece of code. I misinterpreted and thought the offer was unsatisfied with the original, appropriate use for a ternary operator ($variable == $x ? $y : $z)

Walkman• 8 years ago

Point 11. is called " return early".

Guidelines for writing readable code

Related Stories