10 ms·

Maslow's Pyramid of Code Review (2015)

132 points by nkjoep 6y ago

auslegung 6y ago
> Code should be 1) correct 2) secure 3) readable 4) elegant 5) altruist This is a great companion to the adage "Make it work, make it right, make it fast." I would map "Make it work" to #1 and "make it right" to #2 - #4. I don't quite see a clear mapping with "make it fast" to any levels of the pyramid, nor #5 with any statement in the adage, though that doesn't mean there are any problems with either.
- barbazoo 6y ago
  Maybe "make it fast" could fall under #4 Make it elegant, as an elegant way to solve a problem could be a way that solves it more efficiently (faster) than others.
  - citrin_ru 6y ago
    Depending on situation it can be part of #1 - make it work. If a GUI application instead of <200ms has many seconds response time it kind of work, but it depends. I would not use a mail client if it takes a few minutes to open an IMAP folder or a message. But I'm OK with waiting much longer when I'm compressing a multi gigabyte file. If a backend change increases resource usage and now you need 3x more servers (VMs e. t. c.) it kind of work, but can push the project out of the budget ...or it can be a killer feature which would be worth to spend money on.
  - Firadeoclus 6y ago
    Mostly I'd agree. It could also fall under #5, as a trade-off between developer time and user time. And sometimes #1, if speed is an essential requirement.
  - 908B64B197 6y ago
    Yes. High performance code is a world of it's own. Readability and elegance is going to be pretty different in that particular domain anyways. But 95% of code doesn't need the absolute best performance anyways.
- nvader 6y ago
  I am partial to the formulation, "Make it correct, make it clear, make it concise, make it fast. In that order." - Wes Dyer
  - auslegung 6y ago
    When I think of “make it work”, I think, “do any hackey thing you need to to get it to work”. Then in the next step, “make it right“, I refactor and clean up. It’s much like writing, where the advice is to write without censoring, then edit later. I think the idea is it’s always easier to edit something that exists, rather than write something correct the first time, or at least try to make it correct the first time. That being said, when I read your quote where it first says “make it correct”, I would skip the first step I mentioned above, which is a very important step. What are your thoughts?
- bironran 6y ago
  > Correct: ... Is it performant enough for this use case? I completely agree with that definition of correct. Code should be performant _enough_ for the use case. It shouldn't strive for the unachievable "infinite performance" or "endless scalability". It should do well now and in the next performance / growth cycle (usually measured in months to a low number of years unless you're in hyper growth). However I disagree with "Secure" being on a different level than "Correct". Or rather, the "release" line passes over the secure. I may not be happy with the structure of the code, but I will never knowingly release insecure code or allow such code to be released if I can help it. The impact of security issues to the bottom line is usually far greater (in both immediate and future terms) than the impact of any non-data-loss inducing functional issue. -- addendum --- Also I miss "being evolution ready" (future proofing). Sometimes you give up on some of the other aspects to make sure your code (and the data it governs) can be evolved should the need arise.
jschwartzi 6y ago
Some of the lower-level questions map more to design review rather than code review. For example if you're asking "Do the domain models cleanly map the real world to reduce cognitive load?" after the code is already written, it's probably too late to make any changes. However you should have some kind of plan and communicate it to your teammates prior to embarking on the kind of code change that creates new "domain models." During their review of your plan they should be making sure the domain model makes sense.
superbaconman 6y ago
I can't say I've ever been through a proper code review. I've watched a few on YouTube, but they seem kinda simplistic. Are there any good resources for shops that don't have existing code review experience?
heisenbit 6y ago
> As in Maslow's pyramid, each layer requires the previous one. It is useless for code that is charging the wrong customer to be readable. Now would you rather have code that is having a bug but is readable or having code that is incomprehensible but afaik. was giving the right answer when last run? The latter is unfortunately just literally a bit away from being wrong and incomprehensible and a total write-off.
- the_af 6y ago
  > Now would you rather have code that is having a bug but is readable or having code that is incomprehensible but afaik I don't know the answer myself, but the "Practical C Programming" book argues that clear code that doesn't work is preferable to unclear code which works but is hard to understand (because of its messiness). This is because you understand what's wrong about the clear but non-working code and therefore you can fix it. I don't know if this maxim works for every situation, but as a general rule it seems ok.
- itsdrewmiller 6y ago
  I'd rather code review the former but by the end of the code review I'd rather end up with the latter, with really good comments.
hyperpallium2 6y ago
No code gets past the "correct" layer, because all software has bugs. Maybe, "correct enough"? How can you review it if it's not "readable"?
- throwawayboise 6y ago
  If nobody in the code review can make sense of the code because of readability issues, that's the result of the review. Send it back for rewrite.
  - hyperpallium2 6y ago
    The pyramid in TFA has "correct" and "secure" beneath (i.e. before) "readable".
- yellowyacht 6y ago
  Instead of "correct", the layer should be "Performs" or "Works for me". Then maybe the next layer should be "Works for others without crashing"
agentultra 6y ago
Correct with respect to what? The answer to, does your code do what you expect it to? is pernicious and difficult to answer if you're not writing specifications, at a certain scope of complexity.
- hinkley 6y ago
  Parallel to a quote from Tony Hoare's Turing award speech, I find it helpful if the code does what it says it does first. You can hide a lot of bugs (including from yourself) by having the code that works quite differently than you would describe it. It's better if you rearrange the code the way you would describe it (and then you don't have to describe it at all).
hackeraccount 6y ago
I'd call #1 minimum viable product and I'd switch 4 and 5 but it's interesting as I thought experiment no matter how much you agree or disagree with it.
snidane 6y ago
It very much depends on which context the pyramid is applied. I see two types of code review typically done 1. mandatory code review as part of automated workflow. Eg. ticket in jira won't be closed or git branch won't be merged to master before someone code reviews. The reviewer does the code review asynchronously from the code author on his own his schedule. Because of time pressure and pressure of being accused of blocking the team's progress, this kind of code review tends to do the bare minimum, focusing only the correctness part of the review maslow pyramid, if at all. Most often the reviewer just points out some cosmetics so as to appear that he actually looked at it. 2. two people sit side by side (or virtually over zoom) and walk through the code together while having a synchronous conversation, asking questions when not understanding something. Higher levels of the code review pyramid can be accessed using this code review style as well as achieving higher level understanding of author's thinking process and proliferation of good practices. I've seen code review type #1 pushed in manager dominated environments, ending up as a formality and a tool of blame. I've experienced code review type #2 among very senior engineers, often organized informally with no managers involved.
1-6 6y ago
DOTADIW. It's easier to climb that triangle when the project is small.
legerdemain 6y ago
Almost all code review fails right out the gate at hurdle #1, "is the code correct." When you're looking at an interleaved diff on Github, you have little ability to see whether the code is broken, especially when the diffing heuristic awkwardly mashes together unrelated functions and declarations. Over a decade of experience with merging enthusiastically approved, hopelessly broken code, I've come to believe that code review won't, and often can't, and isn't really designed to find bugs, only nits. And this pertains to both local bugs (this function is wrong) and systemic bugs (this function makes false assumptions about surrounding code). What visual code review can uncover: - style nits - a lack of unit tests - feelings of dread when the diff touches a critical part of the codebase What actually uncovers bugs: - thorough tests - in-person code walkthroughs - design documents
- imoverclocked 6y ago
  Thorough tests can also be in the form of good static analysis tools. eg: in Java codebases, errorprone+NullAway can block some really obvious issues leading to NPEs. In my experience, the issue is often getting the codebase to a point where the tools are useful. The simpler (more nit-picky) rules in such system are usually easier to enforce while the more useful ones build on the simpler rules and flag design issues.
  - legerdemain 6y ago
    NPEs are a red herring. My immediate read on people who complain about NPEs is that they haven't done any enterprise Java development. Let's say you have the potential for some reference to be null. And you cleverly wrap it in an Optional or add some @NotNull annotation to it. Great, now how do you handle the null? Maybe it's some query result set that unexpectedly contains zero results or an empty list of live service shards. What do you do to recover? Print an error to the application log and continue running in a corrupted state? Is that really an improvement? Bugs don't get fixed by adding Optional everywhere or "null coalescing" or whatever, they get fixed by actually understanding how the bug arises during operation. It's a bug in the mental model of how the application works, not at the syntactic level of null references.
notacoward 6y ago
The most important part is easy to miss in the intro. > each layer requires the previous one In my experience (several projects and companies) most code reviewers pay scant attention to correctness, ignore security altogether, and spend all of their time on readability/elegance. Time after time after time, I've seen several people have lengthy exchanges about these "higher level" concerns during code review, the code gets merged, and then multiple bugs end up tracing back to fairly basic logic errors that they all overlooked. Why? Because it's easier for people to talk about the superficial structure of the code. It's almost easy to argue about various micro-optimizations (which usually don't even matter). Making sure that each path leads to a reasonable result and/or gets tested is much harder. Identifying the paths/cases that are missing altogether is harder still, as it requires context about the rest of the system as well as the bits under review. Most code reviews are looking for the keys under the lamp post. IMO the only way to fix that is to add some accountability, but that usually gets mistaken for adding hierarchy and process so engineers (particularly the "move fast" variety) strongly resist it.
- firebaze 6y ago
  Sorry, but I can’t confirm your experience at all. Maybe I’m just lucky with my past jobs, but, aside from very junior reviewers, I never observed the „focus on style“ effect. Sometimes it comes as an addendum (like „and please fix the order of imports“), but it is very rarely the core message.
  - why_Mr_Anderson 6y ago
    I'm pretty sure what my response would be if someone rejected my commit with message 'fix the order of imports'...
    - rzzzt 6y ago
      Using the menu item for automatically organizing imports, or the equivalent keyboard shortcut, surely.
  - BlargMcLarg 6y ago
ac42 6y ago
Except for the "Elegant" thing, which I would drop entirely, I think the pyramid either has it backwards or should be read from top to bottom for priority. Usually nobody cares for unreliable code because the "correct" bit can't be figured in most cases. And people messing with the code base kind of just failed the fizzbuzz test.
- ac42 6y ago
  s/unreliable/unreadable/ (autocorrection got me wrong)
  - hinkley 6y ago
    You could say it's unreliable.
rmk 6y ago
The trouble with the 'pyramid' is it doesn't take into account interdependencies. For example, it is very important for code to be clean and understandable in order to ascertain correctness (to the extent possible via code inspection). And the whole notion of 'value', which is subjective, typically dominates code reviews. The 'value' question is basically whether the change must be rethought from the ground up to pass muster as a net value add to the customer. For example, if the code is extremely dense and complicated but essentially correct, then the value may be very high for an earlier stage product, versus for a larger, established product where long-term maintainability costs can overshadow the value derived from the change.
contingencies 6y ago
Recognising that the majority of software life is in maintenance, there's a case for [more] readable > [perfectly] correct.
tinkertamper 6y ago
I bring up Maslow a lot at work, I think it’s one of the few things I enjoyed from my partial business degree. What’s missing from this article is the context of the release, project, company etc. Some project environments don’t allow for the proper time to get the things done “properly”. Some product managers don’t have a clear enough vision or handle on the details to allow developers to know what “correct” really is. If the team, project, company or whatever is not in a good place you can never really hit these higher “levels”; you’ll never reach the next rung on the pyramid if the context it’s built on is still at a lower one.
jariel 6y ago
There needs to be several layers below 'correct'. If we can just get to 'correct' the world would be already so much improved.
k_sze 6y ago
Maybe I'm just weird, but I believe that readable code is a prerequisite for correctness and security. Some people would argue that readable code is not needed for correctness and security if you have good test coverage. But I'd argue that because the tests are also written by humans, how do you know you are testing the right things, and testing correctly, if your test code isn't readable? Ultimately, that judgement has to come from humans reading the test code and the program code.
Hithredin 6y ago
Shouldn't the pyramid be reversed? Surely "readable" come first, then you can review the correctness
codeapprove 6y ago
This is a great article and I'd like to shamelessly plug my product, which is designed to make code review better for teams that work on GitHub You can look at https://codeapprove.com https://codeapprove.com for an idea of where we're going but really if your team does code review on GitHub and wants to get better just email me at sam@habosa.com and we can talk.

132 points by nkjoep 6y ago

> Code should be 1) correct 2) secure 3) readable 4) elegant 5) altruist This is a great companion to the adage "Make it work, make it right, make it fast." I would map "Make it work" to #1 and "make it right" to #2 - #4. I don't quite see a clear mapping with "make it fast" to any levels of the pyramid, nor #5 with any statement in the adage, though that doesn't mean there are any problems with either.

barbazoo 6y ago
Maybe "make it fast" could fall under #4 Make it elegant, as an elegant way to solve a problem could be a way that solves it more efficiently (faster) than others.
- citrin_ru 6y ago
  Depending on situation it can be part of #1 - make it work. If a GUI application instead of <200ms has many seconds response time it kind of work, but it depends. I would not use a mail client if it takes a few minutes to open an IMAP folder or a message. But I'm OK with waiting much longer when I'm compressing a multi gigabyte file. If a backend change increases resource usage and now you need 3x more servers (VMs e. t. c.) it kind of work, but can push the project out of the budget ...or it can be a killer feature which would be worth to spend money on.
- Firadeoclus 6y ago
  Mostly I'd agree. It could also fall under #5, as a trade-off between developer time and user time. And sometimes #1, if speed is an essential requirement.
- 908B64B197 6y ago
  Yes. High performance code is a world of it's own. Readability and elegance is going to be pretty different in that particular domain anyways. But 95% of code doesn't need the absolute best performance anyways.
nvader 6y ago
I am partial to the formulation, "Make it correct, make it clear, make it concise, make it fast. In that order." - Wes Dyer
- auslegung 6y ago
  When I think of “make it work”, I think, “do any hackey thing you need to to get it to work”. Then in the next step, “make it right“, I refactor and clean up. It’s much like writing, where the advice is to write without censoring, then edit later. I think the idea is it’s always easier to edit something that exists, rather than write something correct the first time, or at least try to make it correct the first time. That being said, when I read your quote where it first says “make it correct”, I would skip the first step I mentioned above, which is a very important step. What are your thoughts?
bironran 6y ago
> Correct: ... Is it performant enough for this use case? I completely agree with that definition of correct. Code should be performant _enough_ for the use case. It shouldn't strive for the unachievable "infinite performance" or "endless scalability". It should do well now and in the next performance / growth cycle (usually measured in months to a low number of years unless you're in hyper growth). However I disagree with "Secure" being on a different level than "Correct". Or rather, the "release" line passes over the secure. I may not be happy with the structure of the code, but I will never knowingly release insecure code or allow such code to be released if I can help it. The impact of security issues to the bottom line is usually far greater (in both immediate and future terms) than the impact of any non-data-loss inducing functional issue. -- addendum --- Also I miss "being evolution ready" (future proofing). Sometimes you give up on some of the other aspects to make sure your code (and the data it governs) can be evolved should the need arise.

jschwartzi 6y ago

Some of the lower-level questions map more to design review rather than code review. For example if you're asking "Do the domain models cleanly map the real world to reduce cognitive load?" after the code is already written, it's probably too late to make any changes. However you should have some kind of plan and communicate it to your teammates prior to embarking on the kind of code change that creates new "domain models." During their review of your plan they should be making sure the domain model makes sense.

superbaconman 6y ago

I can't say I've ever been through a proper code review. I've watched a few on YouTube, but they seem kinda simplistic. Are there any good resources for shops that don't have existing code review experience?

heisenbit 6y ago

> As in Maslow's pyramid, each layer requires the previous one. It is useless for code that is charging the wrong customer to be readable. Now would you rather have code that is having a bug but is readable or having code that is incomprehensible but afaik. was giving the right answer when last run? The latter is unfortunately just literally a bit away from being wrong and incomprehensible and a total write-off.

the_af 6y ago
> Now would you rather have code that is having a bug but is readable or having code that is incomprehensible but afaik I don't know the answer myself, but the "Practical C Programming" book argues that clear code that doesn't work is preferable to unclear code which works but is hard to understand (because of its messiness). This is because you understand what's wrong about the clear but non-working code and therefore you can fix it. I don't know if this maxim works for every situation, but as a general rule it seems ok.
itsdrewmiller 6y ago
I'd rather code review the former but by the end of the code review I'd rather end up with the latter, with really good comments.

hyperpallium2 6y ago

No code gets past the "correct" layer, because all software has bugs. Maybe, "correct enough"? How can you review it if it's not "readable"?

throwawayboise 6y ago
If nobody in the code review can make sense of the code because of readability issues, that's the result of the review. Send it back for rewrite.
- hyperpallium2 6y ago
  The pyramid in TFA has "correct" and "secure" beneath (i.e. before) "readable".
yellowyacht 6y ago
Instead of "correct", the layer should be "Performs" or "Works for me". Then maybe the next layer should be "Works for others without crashing"

agentultra 6y ago

Correct with respect to what? The answer to, does your code do what you expect it to? is pernicious and difficult to answer if you're not writing specifications, at a certain scope of complexity.

hinkley 6y ago
Parallel to a quote from Tony Hoare's Turing award speech, I find it helpful if the code does what it says it does first. You can hide a lot of bugs (including from yourself) by having the code that works quite differently than you would describe it. It's better if you rearrange the code the way you would describe it (and then you don't have to describe it at all).

hackeraccount 6y ago

I'd call #1 minimum viable product and I'd switch 4 and 5 but it's interesting as I thought experiment no matter how much you agree or disagree with it.

snidane 6y ago

It very much depends on which context the pyramid is applied. I see two types of code review typically done 1. mandatory code review as part of automated workflow. Eg. ticket in jira won't be closed or git branch won't be merged to master before someone code reviews. The reviewer does the code review asynchronously from the code author on his own his schedule. Because of time pressure and pressure of being accused of blocking the team's progress, this kind of code review tends to do the bare minimum, focusing only the correctness part of the review maslow pyramid, if at all. Most often the reviewer just points out some cosmetics so as to appear that he actually looked at it. 2. two people sit side by side (or virtually over zoom) and walk through the code together while having a synchronous conversation, asking questions when not understanding something. Higher levels of the code review pyramid can be accessed using this code review style as well as achieving higher level understanding of author's thinking process and proliferation of good practices. I've seen code review type #1 pushed in manager dominated environments, ending up as a formality and a tool of blame. I've experienced code review type #2 among very senior engineers, often organized informally with no managers involved.

1-6 6y ago

DOTADIW. It's easier to climb that triangle when the project is small.

legerdemain 6y ago

Almost all code review fails right out the gate at hurdle #1, "is the code correct." When you're looking at an interleaved diff on Github, you have little ability to see whether the code is broken, especially when the diffing heuristic awkwardly mashes together unrelated functions and declarations. Over a decade of experience with merging enthusiastically approved, hopelessly broken code, I've come to believe that code review won't, and often can't, and isn't really designed to find bugs, only nits. And this pertains to both local bugs (this function is wrong) and systemic bugs (this function makes false assumptions about surrounding code). What visual code review can uncover: - style nits - a lack of unit tests - feelings of dread when the diff touches a critical part of the codebase What actually uncovers bugs: - thorough tests - in-person code walkthroughs - design documents

imoverclocked 6y ago
Thorough tests can also be in the form of good static analysis tools. eg: in Java codebases, errorprone+NullAway can block some really obvious issues leading to NPEs. In my experience, the issue is often getting the codebase to a point where the tools are useful. The simpler (more nit-picky) rules in such system are usually easier to enforce while the more useful ones build on the simpler rules and flag design issues.
- legerdemain 6y ago
  NPEs are a red herring. My immediate read on people who complain about NPEs is that they haven't done any enterprise Java development. Let's say you have the potential for some reference to be null. And you cleverly wrap it in an Optional or add some @NotNull annotation to it. Great, now how do you handle the null? Maybe it's some query result set that unexpectedly contains zero results or an empty list of live service shards. What do you do to recover? Print an error to the application log and continue running in a corrupted state? Is that really an improvement? Bugs don't get fixed by adding Optional everywhere or "null coalescing" or whatever, they get fixed by actually understanding how the bug arises during operation. It's a bug in the mental model of how the application works, not at the syntactic level of null references.

notacoward 6y ago

The most important part is easy to miss in the intro. > each layer requires the previous one In my experience (several projects and companies) most code reviewers pay scant attention to correctness, ignore security altogether, and spend all of their time on readability/elegance. Time after time after time, I've seen several people have lengthy exchanges about these "higher level" concerns during code review, the code gets merged, and then multiple bugs end up tracing back to fairly basic logic errors that they all overlooked. Why? Because it's easier for people to talk about the superficial structure of the code. It's almost easy to argue about various micro-optimizations (which usually don't even matter). Making sure that each path leads to a reasonable result and/or gets tested is much harder. Identifying the paths/cases that are missing altogether is harder still, as it requires context about the rest of the system as well as the bits under review. Most code reviews are looking for the keys under the lamp post. IMO the only way to fix that is to add some accountability, but that usually gets mistaken for adding hierarchy and process so engineers (particularly the "move fast" variety) strongly resist it.

firebaze 6y ago
Sorry, but I can’t confirm your experience at all. Maybe I’m just lucky with my past jobs, but, aside from very junior reviewers, I never observed the „focus on style“ effect. Sometimes it comes as an addendum (like „and please fix the order of imports“), but it is very rarely the core message.
- why_Mr_Anderson 6y ago
  I'm pretty sure what my response would be if someone rejected my commit with message 'fix the order of imports'...
  - rzzzt 6y ago
    Using the menu item for automatically organizing imports, or the equivalent keyboard shortcut, surely.
- BlargMcLarg 6y ago

ac42 6y ago

Except for the "Elegant" thing, which I would drop entirely, I think the pyramid either has it backwards or should be read from top to bottom for priority. Usually nobody cares for unreliable code because the "correct" bit can't be figured in most cases. And people messing with the code base kind of just failed the fizzbuzz test.

ac42 6y ago
s/unreliable/unreadable/ (autocorrection got me wrong)
- hinkley 6y ago
  You could say it's unreliable.

rmk 6y ago

The trouble with the 'pyramid' is it doesn't take into account interdependencies. For example, it is very important for code to be clean and understandable in order to ascertain correctness (to the extent possible via code inspection). And the whole notion of 'value', which is subjective, typically dominates code reviews. The 'value' question is basically whether the change must be rethought from the ground up to pass muster as a net value add to the customer. For example, if the code is extremely dense and complicated but essentially correct, then the value may be very high for an earlier stage product, versus for a larger, established product where long-term maintainability costs can overshadow the value derived from the change.

contingencies 6y ago

Recognising that the majority of software life is in maintenance, there's a case for [more] readable > [perfectly] correct.

tinkertamper 6y ago

I bring up Maslow a lot at work, I think it’s one of the few things I enjoyed from my partial business degree. What’s missing from this article is the context of the release, project, company etc. Some project environments don’t allow for the proper time to get the things done “properly”. Some product managers don’t have a clear enough vision or handle on the details to allow developers to know what “correct” really is. If the team, project, company or whatever is not in a good place you can never really hit these higher “levels”; you’ll never reach the next rung on the pyramid if the context it’s built on is still at a lower one.

jariel 6y ago

There needs to be several layers below 'correct'. If we can just get to 'correct' the world would be already so much improved.

k_sze 6y ago

Maybe I'm just weird, but I believe that readable code is a prerequisite for correctness and security. Some people would argue that readable code is not needed for correctness and security if you have good test coverage. But I'd argue that because the tests are also written by humans, how do you know you are testing the right things, and testing correctly, if your test code isn't readable? Ultimately, that judgement has to come from humans reading the test code and the program code.

Hithredin 6y ago

Shouldn't the pyramid be reversed? Surely "readable" come first, then you can review the correctness

codeapprove 6y ago

This is a great article and I'd like to shamelessly plug my product, which is designed to make code review better for teams that work on GitHub You can look at https://codeapprove.com https://codeapprove.com for an idea of where we're going but really if your team does code review on GitHub and wants to get better just email me at sam@habosa.com and we can talk.