Guidelines for writing readable code(alemil.com)
alemil.com
Guidelines for writing readable code
https://alemil.com/guidelines-for-writing-readable-code
100 comments
And the second should, imo, not be a comment, but rather a function name.
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
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
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]
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]
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.
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).
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.
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...
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 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.
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.
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.
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.
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:
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:
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
...
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.
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 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.
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.
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.
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.
"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.
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.
+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.
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.
[deleted]
> 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
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
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.
Or worse, you need to change some behavior and it it only gets changed in one of those places...
If you have dozens of instances of nearly identical code and not a single person knows of more than 2 instances of them, you've got much bigger problems.
This would be true if everyone on the team was equally involved from the very start on a greenfield project, and they aren't all already fighting other forms of tech debt.
Sadly the real world isn't so accommodating.
Sadly the real world isn't so accommodating.
Typically, in the teams I've been on that did the best job of keeping the code clean, we'd handle this by just being good at code review: The person submitting the change might not remember any duplicates, but there's a good chance that one of the reviewers will, if that's one of the things they're watching for.
Alternatively, if you really want to be exacting about this, there are code analysis tools that will do it for you.
Alternatively, if you really want to be exacting about this, there are code analysis tools that will do it for you.
If your code base is this big and you're adding a new feature, you really should be doing an impact analysis prior to making any changes.
So, instead of dozens of copies of a snippet (which can typically be identified with tooling), the repo ends up with half as many dozens of partial abstractions? I guess it could also end up with a Frankenstein's Monster of an abstraction with more arguments and conditionals than the original code.
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.
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.
Couldn’t agree more about the negative impact of this rule on a big team / large old codebase. The “copy once” rule pushes back against abstractions rather than the real problem of wrong abstractions, too big abstractions, or too complex interfaces to abstractions.
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.
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.
I think that's okay, because those large, multi-person codebases are precisely the ones which pay a huge cost for premature abstraction.
Too much copy-pasta is a great reason to refactor.
Too much copy-pasta is a great reason to refactor.
That's why you have your code reviewed by other people in the team before it hits master.
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.
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.
I agree, nothing is a hard and fast rule. And sometimes, an attempt to generalize copied-but-customized code can result in much less readable code than just leaving the originals in place.
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
> Code duplication is far cheaper than the wrong abstraction.
https://youtu.be/8bZh5LMaSmE
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.
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.
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.
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.
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.
How does checking at the start not convey what conditions are relevant and what assumptions can be made in the rest of the function?
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.
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"?).
#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.
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.
To add another perspective to the discussion, Yegor Bugayenko (author of the Elegant Objects books) says that splitting data and data manipulating objects are "pure evil" because when you do it that way it is going against OOP as you will get functions and data to operate on. Which is exactly where we were before OOP.
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... :)
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... :)
I haven't read his book, so I can't speak to his arguments, but I did want to say that I don't think that doing so puts us exactly where we were before OOP.
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.
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.
What I always ask is, why use classes at all? I love that newer language docs like the ones for Kotlin and Swift are telling people to use structs and bare functions.
To name one example:
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.
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.
You are mixing different concerns. The Haskell example you are talking about is just interfaces, not classes. As for data abstraction, we can use modules; either the weak Haskell ones or better ML ones. Classes simply conflate the two concepts. I'd rather remove the feature of classes and introduce those two instead.
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.
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.
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.
* 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.
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.
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”.
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”.
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.
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.
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).
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).
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.
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.
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)
you dont reduce nesting by moving all checks to the beginning, you reduce nesting by moving the contents into methods.
if (product.hasPrice){ if (someOtherCondition){ Cart cart = cartService.getCartForCurrentUser(); cartService.addToCart(product,1); } }
to
if(canSellProduct(product)){ addToCurrentCart(product) }
if (product.hasPrice){ if (someOtherCondition){ Cart cart = cartService.getCartForCurrentUser(); cartService.addToCart(product,1); } }
to
if(canSellProduct(product)){ addToCurrentCart(product) }
> 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?
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?
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.
That's an interesting point. I'm not entirely sure I agree with that reasoning for early returns, though.
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.
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.
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
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.
> the following lines could assume any combinations of those conditions.
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?
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?
I just soak everything in asserts.
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.
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.
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.
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.
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 :).
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 :).
> I honestly cannot think of a reasonable argument for why constant values should not be in static constants or enums.
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:
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);
I will say your take is reasonable, but on projects I manage I still push for usage of const/enum in these cases.
I'm actually arguing that we should not be focused on nitpicking the details, and instead appreciating the effort taken to think things through, and supporting the idea of using the article as a springboard for our own improvements.
[deleted]
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...
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...
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...
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...
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...
> 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...
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
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
Isn't this #9 Prefer internal functions over custom solutions?
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.
I don't like the look of #10. I'm a believer in single-point-of-return.
> `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.
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.
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...
https://www.youtube.com/watch?v=rFejpH_tAHM
https://talks.golang.org/2015/simplicity-is-complicated.slid...
Thanks, I will.
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?
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
But people use it for just looping through an array without creating a new one
Oh, the irony.
(Yeah I know this is not good HN ettiquete, but I just could not resist in this case. Sorry.)
(Yeah I know this is not good HN ettiquete, but I just could not resist in this case. Sorry.)
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.
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.
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.
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.
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.
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!
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!
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.
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.
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)
“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.
The primary reason to avoid code duplication is to improve maintainability and refactorability, not to improve readability.
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.
I'd add, beside obvious docstrings, comment to explain why the code does something when it's not obvious (say a corner case).
Point 11. is called " return early".
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.
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.
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.
Of course! I am talking about the names of local variables, not the names of externally visible functions or global variables.
There's a spectrum of how 'local' a variable is though. There's a big difference between a local variable whose uses span ~5 lines vs. ~30 lines. You're making a trade-off between repeating information (what the variable is) and forcing the reader to keep in their own (human) memory what each variable is for (or else forcing them to constantly go back and re-discover what the variable is for).
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.
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.
This sounds more a problem about your total number of local variables than their names. If you have too many variables, using longer names is not going to help you as much as organizing your code differently.
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.
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.
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).
Remembering 10 single letter mappings vs clear variable names sounds onerous.a, b, c, d, e, f, g, h, i, and j vs price, discount, coupon, item_description, item_id, ...
> Remembering 10 single letter mappings vs clear variable names sounds onerous
Have you ever read an applied math book? They do not seem to have this problem.
My point is that
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;
> Have you ever read an applied math book? They do not seem to have this problem.
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.
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.
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.