Maslow's Pyramid of Code Review (2015)
dein.fr
dein.fr
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
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.
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.
All that being said, the complaints about NPEs have always felt to me like focus on symptoms (null references) rather than the underlying causes (badly-designed application code)
If something is labelled @Nonnull (or @NotNull, depending on the annotations used) then it should never be allowed to be null. The issue arises when there is no enforcement of this paradigm at the compiler level. Similar languages (Kotlin/Swift/...) with the benefit of Java in hindsight, can track nullability as a core feature. Java requires a plugin to track+enforce nullability for backward compatibility reasons.
errorprone is a Java compiler plugin to add extra sanity checks that the base compiler doesn't/can't provide. NullAway is an errorprone plugin and has been shown [1] to get rid of NPEs.
In my experience, anyone who thinks NPEs are unavoidable or even a wanted feature has not sufficiently learned about nullability as a language/type system feature for type systems that allow null values.
NullAway isn't the only game in town (for Java) but it's the simplest and easiest model to adopt for a large body of developers that don't necessarily want to think hard about null values and complex type interactions.
Finally, I'm speaking from first hand experience of wrangling a large Java codebase with many developers. The mental model of the code is far too complex to hold in any one head. Even just the mental model of nullability is too complex to hold in a single head. The good news is that this is the kind of problem a compiler can excel at... and that a PR check can leverage.
The flip side of course is that the original coder does still need to be reasonably competent. Relying on even exhaustively thorough and repeated code reviews to turn basically garbage into reasonable quality code is a bad bet.
Correctness is one competent dev banging on the code until it works, and then getting git-blames and bug reports sent directly to them when it stops working. We don't blame the reviewer for our own bugs. If the dev leaves, it's faster for the next dev to paper over the bug or rewrite the feature than it is to do a close reading of the existing codebase. That's what "rapid iteration" and "agile development" is all about.
Expecting reviewers to checkout and run/test things is such a time sink, and over the course of a year of doing it at a new work place, I've never seen it be the thing that caught a bug. If anything I feel like it caused more bugs because people end up getting laxed with tests.
I have to say, now over 30 years as a developer, I've only been introduced to code reviews in the last few years.
Somehow, I have no idea how, we shipped code for nearly 30 years without code reviews or unit tests.
Thank god we now no longer ship with bugs.
Code reviews are not a silver bullet, and we still released some whoppers, but halving the deployed bug count and improving the code base is a worthwhile outcome.
We’ve recently started doing CD with E2E tests and I reckon E2E has halved the deployed bug count again. It’s definitely stopped bugs from entering production.
There always seems to be a trade off between the cost of action (in resources, time to release, flexibility) and quality, there is no right answer (despite what people will tell you), and it’s up to you to set the acceptable bar for released bugs, and to deal with the consequences.
For me, code reviews improved bug counts less than I’d expected but had other quality benefits that I felt more than made up for the modest cost of the additional ceremony.
It is much easier to share friendly hints and observations while writing the code. When the code is written it is difficult to show how it could have looked or demonstrate the process of getting there.
And so even though my org requires us to conduct code reviews (and I find them helpful, at a hefty cost), I will still ask people to pair up with me regularly.
This to get to know the person better, to see what the problem is (and sometimes I am the problem), to help work out better design, etc. All of which is difficult with regular review.
It was a great way to work, because junior people could get up to speed quickly, and it was much less likely that would would get stuck on any problem. As a bonus, we didn't need code reviews because every line of code had already been seen (and if necessary, critiqued) by another person.
One drawback was that you could not start working until your partner for the day arrived, and you'd have to stop working when the first person of a pair left for the day.
I'm not sure how this way of working pays off during the annual performance reviews, I didn't stick around long enough to find out.
That being said, I think impersonal reviews are still needed. You don't always have time for the soft human touch, especially at a smaller startup. The defensiveness some people have should really only ever be a short term problem. I've worked with people like that and found that being persistent, objective (follow standards that you make everyone aware of) and consistent with reviews fixed that over time. Rapport building is good , but you can't require it every time, so you need the other end of the spectrum too.
im hardly doing the bit justice because it's truly an all-timer-- highly recommend the whole thing, beyond just that bit of it (which i think comes early on iirc): https://www.youtube.com/watch?v=kGlVcSMgtV4
https://stackoverflow.com/questions/2898571/basis-for-claim-...
I agree. Is everybody writing and running their own bespoke testing on each PR, with no cross-person commonality, in which case why is your code so complicated that they have to do this? Or is everybody doing the same kind of thing every time, in which case why don't you automate that and run it in a CI process?
IMO, you can only really apply one ideological principle to your practices. I think that should be that your code works correctly while being no more complex than needed. Ideally many things shouldn't need to be done, but in the end, you do what you need to do to make sure that's true, including reviewers checking out code and working with it. If your ideological principle is instead that reviewers must never checkout code and try it, then you will eventually sacrifice the other in favor of it. I don't think that's a good result.
Nits are, both literally and figuratively, bugs that just haven't hatched yet.
(Relevantly, the things you’ve pointed out that code review can identify are all issues that impact maintenance of the code, including the ability to find bugs and avoid creating them.)
Also, changesets that are large but can't be broken down into smaller ones because of practical constraints cause 'code review fatigue': people will review the first few commits/files with gusto and lose steam by the time they are reading the critical code which may arrive in a later changeset or file!)
Most of my code review comments are along the lines of "what does this do" -- the primary question I'm asking in an in-person review. My goal for a review is to pre-emptively answer the questions I'd ask if I had to fix bugs in the code. This frequently exposes bugs because it requires the author to consider their code from a different perspective.
Maybe it's the code under review: I work in games and was reviewing a lot of code from juniors, so I'd often be asking questions intended to get them thinking about their technical design and we had very little rigor.
In my eyes, the biggest upside of in-person is how it reduces mental drain from back-and-forth. A conversation is comfortable instead of the digital equivalent of repeated red ink all over your work.
> Over a decade of experience with merging enthusiastically approved, hopelessly broken code
Or maybe you're commenting on the code reviewers you've worked with rather than your own experience as a reviewer?
https://www.amazon.com/Making-Software-Really-Works-Believe/...
IIRC the evidence was that formal code reviews were no more effective than async peer review (but both were effective at uncovering bugs).
* naming this variable `accumulator` instead of `x` more clearly communicates intentn * add a quick comment explaining the reason this workaround exists
etc.
A major focus of code review for me is the changes are maintainable in the sense that they empower people to make changes in the future effectively.
I can agree with the post though that correctness and security are more important technically.
> What actually uncovers bugs: > - thorough tests
How do you know that the tests don't have bugs?
> - in-person code walkthroughs
How is this different from reading the code in a code review, or checking it out on your own machine and reading it there?
> 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.
Sometimes it comes as an addendum (like „and please fix the order of imports“), but it is very rarely the core message.
In the process of addressing this kind of feedback it's pretty common for very local correctness issues to be discovered. Oops, used greater-than instead of greater-or-equal. Oops, didn't check for the right error code (or any). Oops, now we need to free this object at a different/additional point. But these discoveries often seem accidental. A lot of low-level errors still slip through, and higher-level logic errors almost never get caught. A perfectly "correct" piece of code for handling disk errors is useless if it's not in the path we reach when a disk error actually occurs. A perfectly "correct" message handler can still invalidate the distributed algorithm of which it's only one part. And so on.
In 30 years, across a dozen companies and half a dozen specialties, I've found that maybe one in ten engineers at "senior" level or above will systematically review others' code for actual correctness - identifying missing cases, making sure the expectations on one end of an API line up with the expectations on the other, finding possible race conditions, etc. They're like gold when you find them. The other nine out of ten typically spend their review time around the periphery instead of addressing the core issue of whether the code does what it's supposed to.
That’s different from style nits, and also why automated stylers and linters are so worthwhile, so your tools handle the stuff you don’t want people wasting their time on.
Unfortunately, most reviews stop there, but at least it’s the right order in a Maslow-esque pyramid.
But as the code size increases, the likelihood that the bug is in this one function goes down. First I have to scan ten functions and prioritize how likely I think it is that the bug is in #2 versus #7. If every one of them is a precious snowflake, then they all have code smells that I have to investigate. It can take me much longer to find the right one, and by then I've lost the memory slots that contain the reproduction steps and have to dump state again to go pull that back in.
"I found what I'm looking for! Now why was I looking for that?"
For me the important thing about code review is bringing in people who may be more familiar with other components with which the code interacts.
When the one who speaks clearly is crazy, you know it right away. With the one who speaks obtusely, who knows how big of a mess they can make before you notice.
Same with code reviews. Clear code is either clearly wrong or typically easier to debug when it isn't. Our jobs are to make code we can maintain, not build monuments to our own magnificence.
If it were the latter, I'd agree 100%, but in the former case, correctness and thoughtful test coverage are actually more important.
Also, reviewing only for correctness is possible even with messy code; it's just a bit harder. I've reviewed code that would melt most programmers' brains, but it had to be structured the way it was because it was performance critical and/or had to fit within tight memory constraints. Dealing with that difficulty is part of the job.
> each layer requires the previous one
So at least the article thinks the hierarchy has order.
And complex topics can be conveyed in a readable way, it's just a bit harder.
You probably think that was a great "checkmate" response but it's quite the opposite. No matter what the author meant, what I meant was quite clear. "Requires" does not necessarily refer to order. Requirements to do X after Y are not uncommon in real life. "If this happens, you must do that" is even codified into some laws. Requirements to do A optionally before B are not uncommon either. Think of a recipe with optional ingredients. You can add pepperoni before you bake a pizza, or not, but if you just add the pepperoni but don't bake you'll get a poor result. The pepperoni-adding requires the baking to produce a useful result, even though the order is reversed.
Just as with code, lifting words out of context often produces a poor result. Reviewers should do better.
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.
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.
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.
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?
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.
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.
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.
How can you review it if it's not "readable"?
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.
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.
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.
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.
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).
You can look at 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.
If we can just get to 'correct' the world would be already so much improved.
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.
(autocorrection got me wrong)