> 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.
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.
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.
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.
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.
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?
> 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.
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.
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?
> 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.
> 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.
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.
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).
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.
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.
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
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.
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.
I agree, but I think the benefit of things like Optional is a bit different: it’s not really about eliminating NPE-type bugs, but limiting the scope in which they can occur. If you use them consistently, you only have to check the behavior of places where the optional is unwrapped, and you push those places upwards towards the entry-point rather than weaving null-checks into your business logic.
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)
Yup, NPEs are not caused in the location that they occur. They are caused by the part of the code that inserts a null where it doesn't belong/isn't expected. This means that getting rid of NPEs requires detailed enforcement across module/method/interface/etc boundaries.
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.
One quick clarification, NullAway inverts the check to enforce, "if something can be null, it needs to be labelled @Nullable" ... it doesn't care at all about @Nonnull annotations. This means that potentially null values are traced accurately through your codebase with @Nullable annotations.
Thanks, this narrative makes sense to me! I'm a little sad never to have worked on a commercial Java codebase that showed this degree of discipline, though.
I'm currently at a place with a culture of thorough code reviews. IMO, it entirely depends on the people and the culture. It's on the reviewer to actually thoroughly review the code and ensure it does what it's supposed to do, including local checkout and testing if needed. And of course on the management structure to provide senior enough reviewers with enough time to do that. It certainly is easier and less time-consuming to just skim it and comment on some style nitpicks and unit tests, but there's no reason it has to be.
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.
At the moderately large, very successful companies where I've worked, code is mostly disposable. It's written and read, on average, once. Code craftsmanship doesn't make any sense in these environments. We treat code review as a formality. Like, we just kind of skim the diff and click the approve button, but most of the time none of us really have the time to understand the code on a deep level.
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.
I am vehemently against making reviewers checkout and test code. You should have tests that demonstrate correctness with very high confidence, and a release process that can find bugs early.
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.
Although I think you’re being sarcastic, I’m sort of in the same camp. We started doing code reviews a few years ago after years of informal releases, and I found they acted like a sieve; I reckon we halved our deployed bug count, at reasonably low cost, but also code style and knowledge improved across the board.
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.
I think you have a point about spreading knowledge. Rather than code review though, I think having engineers pair up on a feature is another (better?)way to knowledge share that probably is more efficient time-wise?
I prefer pairing up. Giving feedback on code that has already been written, and by more junior member of the team, is fraught with problems. People get defensive thinking they have already completed the task and now I am delaying the closure.
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.
For a while I worked on a team where we wrote all code in pairs. The team room had 4 computers, each of which had 2 pairs of 2 monitors and 2 mice and 2 keyboards. During the day, every hour or so, each dev would switch between driver and passenger roles.
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.
I am all for pairing up and interacting with people directly and in a human manner. It's good for building rapport and trust so that things run smoother in every facet.
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.
Rich Hickey nails this hilarious in his (amazing) "simple made easy" talk from 2011 where he calls this mentality guard rail driven development: we don't drive down the highway careening back and forth off the guard rails as we drive down the road, thinking "thank goodness these rails are here or i wouldn't be able to drive down this highway"-- why do we think this about our unit tests?
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
This boggles my mind. I checkout and manually test every PR that I review, and the vast majority of the time (> 80%) I do find a bug. In fact, I'm very suspicious whenever my PR's get the green checkmark without any other feedback.
> Expecting reviewers to checkout and run/test things is such a time sink
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?
To be clear, there definitely should be unit tests run via a CI system. That's a minimum bar instead of a cure-all though. What checks that there are enough unit tests to cover appropriate cases, but not so many as to be noisy or excessively time-consuming? What checks that the tests actually assert the correct behavior? Code review does. What do you do when your test suite takes multiple hours to run, but still doesn't catch all of the bugs?
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.
> 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.
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.)
Yeah context plays a big part here. We wanted to make the experience of looking at "interleaved diff[s] on Github" a lot better. So we made https://www.codestream.com/ which lets reviewers look at pull requests (github) and soon merge requests (gitlab) right in their editor, leveraging the entire context of the file and all the things they love about their IDEs.
This is very much on the money. Code reviews often fall into the trap of people discussing and dissecting things that they are comfortable with, while ignoring the difficult aspects. For instance, reviewers often dissect the elegance and variable naming to death while totally ignoring how the change has been tested (unit tests can show some issues, but if the tests themselves are not covering the core aspects of the problem being solved, then they are not hugely useful, particularly in duck-typed languages like python).
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!)
On my current team, we use code reviews as more of an RFC mechanism. There’s a lot of trust in placed in PR authors to decide, based on delivery constraints, whether comments raised on a review are blocking or not. For large diffs that aren’t practical to review line by line, we tend to use narrative-based architecture/design reviews instead.
Why do you think in-person code walkthroughs are effective but tool-mediated reviews are not?
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?
I always thought the primary value of code reviews was informational, not bug finding. That is by reviewing code you have a better understanding of the changes that went into it, give you a chance to ask why something was done and will generally lead to better future interactions with it.
it's often a good opportunity to point out things that aren't clear when first seeing the new code. Like:
* 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.
Yeah, I would argue that in-person code walkthroughs are worse than reviewing someone's code in isolation. Once it's merged in, the author isn't going to be there to explain what the code is doing here and there.
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.
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.
It depends on the individuals and the places. I can attest to GP's situation being predominantly prevalent in my life. Stylistic issues can be as simple as passing different data on the backend rather than cleaning them up on the frontend, without any mention why one is preferred over the other. I've had cases where people wanted me to make a new variable Y in a model to pass 1 - X, where X was another float variable being passed at the same time (so we passed both Y=1-X and X without intuitive or clear reason). Or saying "this needs a comment" (notice the tone too, it's not "can you add a comment here", or better, "this reads kind of iffy, can you improve it somehow?"). Things a linter or guidelines won't pick up.
The fact that you mention "style" when I never did suggests that we're working from different definitions. I had originally typed a sentence about not meaning stuff like variable names or indentation or (as you mention) import order, but I deleted it for the sake of brevity. What I mean by focus on readability/elegance is stuff like moving code into separate functions, using structs/enums/optionals instead of long argument lists and opaque booleans, etc. It's still valuable, but it's not correctness.
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.
I suppose what you want is possible provided the reviewer gets as deeply invested in the process as the author. Most of the time we're lucky if we even know enough about the project to understand what the author is trying to do - in fact I noticed the article claimed that the requirements should be self-evident in the programming, which I think is ridiculous.
I think all the time on readability is worthwhile. It sucks to go into an unfamiliar codebase, and that includes your own code you haven’t touched in six months. Making maintenance and usability easy comes first. Once the code is understandable, you can start working on correctness. If you can’t reason about it easily, you can’t get trustworthy correctness. Just like a pyramid implies with readability/understandability at the bottom.
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.
I maintain that part of the disconnect here is that people assume you're looking at the correct piece of code. They defend it being not a big deal that it's arcane because it fits on one page and with a little work you can understand it.
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?"
Which is why I am partial to mandatory coding rules, preferably enforced by tooling. Once the conversation about style is eliminated altogether, people can focus on the important things.
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.
Time after time I've seen teams pick the articulate candidate over the inarticulate one.
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's not readable, how are you supposed to ascertain correctness? IMO, readability is step one, and without it you don't have a foundation to be able to make judgement calls on the other aspect.
Typically 'readable' winds up meaning 'conforming to local hyper-specific style foibles' rather than 'all your variables are named a, b, c and you didn't indent anything'.
If it were the latter, I'd agree 100%, but in the former case, correctness and thoughtful test coverage are actually more important.
Outside of stupid format nits and other localized formatting rules, you can have a structure of the code that isn't conducive to being read. Just like a book can follow all of the right guidelines but be a slog to get through, a program or patch can be structurally difficult to consume by reading. That's the main part I'm addressing. Stupid formatting bikeshedding should be staved off with an automatic checker and not make it's way into a review, but that doesn't address readability as a whole.
That's a fair point, but I don't think the hierarchy is really about order. Perhaps you should review for readability first, at least sometimes. It's perfectly reasonable to ask for readability changes before continuing, but if reviewers stop there without going on to review for correctness then IMO they have failed to perform their duty. By contrast, if reviewing for correctness is all reviewers do, it's still better than if all they do is address readability (though better still if they do both). Doing X to facilitate Y doesn't necessarily mean X is more important than Y.
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.
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.
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.
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.
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.
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.
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 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.
82 comments
[ 4.3 ms ] story [ 169 ms ] threadThis 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"?
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).
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.
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.
[1] https://arxiv.org/pdf/1907.02127.pdf
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.
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)
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.
If we can just get to 'correct' the world would be already so much improved.
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.
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.