The Code Review Pyramid
morling.dev
morling.dev
The way this has worked successfully for me in the past is as a new joiner to a team you submit for code review every time. My first couple of reviews tend to have a good bit of feedback as I settle in to the style of the team and often a new language or framework. After a while though I become a domain expert for the projects I am working on, and code reviews end up becoming a rubber stamp. At that point it's just something slowing me down. I'd request review when I need it, or I'm making a big change that will effect everyone, otherwise there are many cases where there is really only one way to skin this cat and it's totally perfunctory.
This sort of arrangement also requires a bit of ownership, if you see reports floating around about an endpoint you just worked on having issues you have to be able to be trusted to jump on it. I feel like trust and ownership are touchy feely people problems so tech people invented code reviews and linters to try to make up for not cultivating the sort of culture where they aren't required on every single little thing.
When we switched to Git we decided that we liked code reviews and wanted to make them part of merging to develop. Depending on the content the reviews are deeper or more superficial. Many times even when I review the code of somebody making changes in their own subject matter domain I have found bugs that simply needed another set of eyes to catch.
I like code reviews and in the context I use them find that they are a complement to trust, not a substitute.
I have never once felt like a code review did anything to erode trust and I don't see how a code review could slow down a work process. Using git, as soon as I submit code for review I begin work on the next task and if I need anything from the branch I submitted for review, I just merge that into the new branch. If changes are requested I make the changes and then merge again.
Code reviews are among the best ways of ensuring that code remains consistent and that everyone is learning from everyone else. That's why in my team all pull requests are reviewed by two people including one junior developer.
Case in point: OP used the word "effect" when he should have used "affect".
Just like pudding, there's always room for a code review.
I'm not sure I agree with your premise that code reviews are the product of a culture with a lack of trust and ownership. I think, in fact, submitting to a review process is a sign of ownership -- it's saying "hey, the quality of the code we produce as a team is important to me, and I know I make mistakes."
Please don't do this. I keep finding rubber stamp code reviews (or none at all) while diving into codebases for where bugs were introduced, and 9 times out of 10, it's someone who didn't bother to get proper reviews for their code because "it's just something slowing me down". I can assure you that you have bugs and usability issues in your code, not just "linter issues". Everyone does.
If your code reviews are becoming rubber stamps, that's an indication that you need to slow down and teach other team members about the system, or hire smarter people who can understand your reviews more quickly. Don't respond by getting rid of the review process as a knee-jerk response to these sorts of organizational constraints.
If you have a practice that isn't effective (e.g. code reviews that aren't preventing bugs), doubling-down is almost never the correct solution.
Code reviews are like unit tests; they're helpful under the right conditions. Mostly on the tricky bits or when a dev is working in an unfamiliar area.
It's a common fallacy for folks to believe that if a little of something is good, then a lot of it must be better. I've been at places with 100% code test coverage or requiring EVERY SINGLE PULL REQUEST to be reviewed. It's obnoxious, and it doesn't do shit to prevent bugs. Instead it causes folks to burn out on the practice and write crap unit tests or stamp "LGTM" on every PR they're forced to evaluate. People have a limited capacity for cognitive engagement and wasting that on piddly crap means they can't spend it on the important bits.
I mean, it's possible that code reviews are worthless. But enough people have found value in them, across enough organizations, enough code styles, and enough decades, that it seems really hard to conclude that. It seems more probable that the team that finds them useless is doing them wrong. (Yeah, I know, No True Scotsman. Still, the first response should be "maybe our team is doing them wrong, and can we fix that?", rather than "let's stop doing them".)
Plus as a peer reviewer, you're often in the same part of the sprint cycle as they are - meaning your mind is already juggling a lot of things about your own code. It's tough to "come up for air" and then immediately dive into someone else's code changes with enough mental capacity to provide a truly useful review.
It's easy to slip into the rubber stamping mentality when you generally trust the ability of a particular contributor.
The other thing I think is easy to do is simply see if you understand the code. If the reviewer struggle to do understand what's going on, chances are that other people will struggle as well. In this case, your lack of knowledge is an asset, the original author is likely to not see how it looks for someone not as well versed with the problem.
Also, for less experienced members, reviewing the changes of more experienced people is a great way to learn if you properly sit down and understand the change (why it’s being done, why it’s being done this way) rather than just skimming through. In addition to catching mistakes, this is useful to check that code is actually comprehensible to people other than the author and a good time to request more/better comments if necessary.
Why do you assume it's a rut? It might be an organizational liability but it can be a great while it's working. It's common folklore that design by committee leads to poor outcomes, or at best average outcomes completed slowly.
Why assume that we need to build code by consensus or a series of interchangeable cogs?
The most convoluted code I ever worked with were in teams that treated code review as secret cow. It happened twice. The best was in teams without code review, but with strong ownership.
The issue is, I think, that code review is myoptic. It ensures surface level readability and does very little for actual structure. And I think it gave people false confidence and made them less willing to refactor.
The problem with unit tests and code reviews is that they ultimately aren't sexy. There's no way to do them that destroys the integrity of the system. Especially when a large percentage of the code written is passable. Yes, reviewing and testing code that works and is bug free is a bit of a waste of time. But you won't know what code isn't working and bug free unless you test and review.
And then there's the fact that most of what we do is not that important. If my code is buggy or inefficient or a third thing, the worst thing that happens is a few people frown while I get it sorted. If you knew that if your code had a bug in it, it would definitely kill 5 - 10 people every time it is run, you'd treat review and testing with the utmost importance because the cost of committing buggy code is way too high.
Testing and review becomes exactly as important as your product.
First and foremost, code reviews are for enforcing team standards, such as testing coverage, documentation, and adhering to scope. "Our code reviews aren't preventing bugs" is not a useful metric in that sense. If you're relying on code reviews to prevent bugs, you have other problems in your development process. If I spot a bug during code review, my first question would be "why wasn't this bug found by the automated test suite?"
Totally agree and well said. In my experience the mentality that code review should catch bugs puts unnecessary pressure on the reviewer which leads to people not wanting to review code, because what if they missed a bug.
My colleagues sometimes find non-trivial errors in my code, and I find errors in theirs. Maybe we're just not as smart as the people you work with.
Some times, they actually hinder the process of fixing bugs.
Have been in some teams with very high junior/senior ratio where a junior would need a whole day to review a small thing a senior implemented in an hour, often because understanding the context leads to hundred rabbit holes of more new information, not the small diff itself. Add the one week that PR was waiting for someone to even notice it needed review and you realize the frustration. Vice versa a junior would take a week to implement something a senior can review in 15mins.
Do the math and you realize the senior will quickly get blocked.
Slowing down and teaching others in that situation is a as you say the right thing to do. But that also saturates after a while. You can only pair program x number of hours per day before getting exhausted, especially for a beginner this can feel like a lot of pressure to ingest so much knowledge at that pace. They need to slow down and get in their own zone as well. In the meantime, backlog is on fire with tasks that need to be released yesterday.
Hire smarter people? Yes please, would love to do that, where do I call? “Smarter” doesn’t always help either, the reality is that you often end up with domain experts within some areas of the product, even if you try to keep everyone on board.
People just pushed code. We occasionally held some review sessions to go over code and exchange comments / ideas about what might be wrong with this or that code and how to prevent this kind of mistake in the future.
But starting from around 2016, I could never ever find a company that does not have mandetory code reviews for every single commit, and this bothers me tremendously.
It's one of the reasons I don't want to work at any programming company anymore.
The really good programmers I know don't look positively at code reviews.
If you need code reviews, ask for it. If not having mandetory reviews results in a huge stream of bugs, maybe you should hire better programmers instead of cheap ones.
Then farther down that continuum someone is busting your chops for some stupid reason that, after you get back from lunch or coffee, you realize is completely valid (or even gives you an idea for an enhancement) and okay you change how a conditional works or add some more tests.
It makes me wonder if in a corporate setting PRs would be better treated as pseudorandom audits, based on a random factor and a weight for automated linting (so people don't learn to game the linter, which can be some of the worst code you ever see.)
A related issue is that most tools make it pretty difficult to look at and interact with old PRs. Feedback loops have some slop, and while it's better that you get the feedback as you are writing the code, fast follow often works as well, and I think we'd be better as a group if you could request revisions on a recently landed PR after the fact. This is mostly okay but you really need to fix your variable names, conditional block, and you missed a boundary condition over here that might not break today, but there are already stories in the backlog that will break it for sure.
1) There is too much knowledge to fit inside one person's head. Cooperation is the keystone of success.
2) Your team will grow. You will need to share this knowledge somehow before the trust can be given
But in defense of this view point: code reviews shouldn't be the opposite of what you're talking about.
I trust my teammates to catch any mistakes I've made. I'm human and therefore fallible. It's going to happen.
I trust mistakes they've made are for the same reasons.
What you're trusting is your teammates are top tier 1% every time and bordering on being super human.
I'm trusting that my team has my back and we're in it together.
With that said, code reviews only work, in my experience, if team members are strong communicators. Understanding each other, empathizing with issues, and being collaborative rather than tearing people down is critical.
Code reviews are a “we” activity not a “your” and “my” activity.
So most important questions are: (1) API must be as correct as possible, especially if its exposed outside of the immediate team. Changing APIs is a royal pain. (2) Documentation of what's going on, especially for non-obvious stuff. If we come back to this in 3 years time and the author has moved on, do we have any hope of fixing it? (3) Testing. What degree of confidence do we have this is doing the right thing? Will it protect me when I change it in a few years time?
Any minor bugs or code style issues can be fixed later as/when they are found and cause problems.
Of all the issues that might be raised in a review, which are the most important to fix? I find it difficult to imagine a ranking in which incorrect functioning (including security vulnerabilities and truly inadequate performance) are not at the top (answering the ranking question with "all of them" would just be a way to avoid the issue.)
In this pyramid, issues of this nature are to be found at all levels except the top one, mixed in with more-or-less subjective ones, such as "Is a new API generally useful and not overly specific?" - pointless flame wars have erupted over issues such as this (also, orthogonally, code review is late in the game to be asking this one.)
For a review to be successful, its participants need to be able to restrain themselves from going down rabbit holes that could not lead to necessary rework - or a strong moderator, if the individual participants cannot act with adequate self-restraint.
If your organization is such that you cannot even discuss these issues until you have working code, then you have other, bigger, problems.
WRT performance, I specifically wrote "truly inadequate" in an attempt to distinguish between breaking and merely undesirably bad performance, but that is a somewhat vague line - unavoidably, I think.
Off the top of my head, the only sort of security 'problem' that can safely be put aside would be one that cannot lead to serious or irreversible harm, or requires the organization to have already been terminally compromised.
That I have to make these distinctions is, in fact, the basis of my concerns. They follow from two premises: firstly, the purpose of an inspection is to find problems that need to be fixed, and secondly, whether a problem falls into that category is orthogonal to the questions of what functional, structural or operational categories it falls in.
I think the foundation of the pyramid (API and Implementation semantics) is often discussed in code reviews when it should have almost no place. By the time you get to the code review not only should these have been ironed out and shared with the whole team. What you should be looking at is simply adherence to the plan, and calling out deviations.
I wrote about a small rat about this: https://xangelo.ca/posts/code-reviews-are-failure/
That's a fair point. It's my background in open-source and I suppose the specific ways we work in the projects I'm involved with which made me add these things at the bottom. Oftentimes, contributors come up with PRs for new features, and there was no prior discussion about API design etc. (although we highly recommend to have such discussion before starting with the work on a large PR).
I.e. oftentimes a PR review will be the first time I see how something is implemented, how the API looks like etc. It may be different with more controlled settings in in-house development teams adhering to more formally defined processes.
Code should be readable. Once code is readable it makes reviewing the rest much easier. Readable code is more maintainable and problems jump out at you.
Stuff other than readability is important, but if you focus on readability it makes the rest of the review go very smoothly.
First pass: Functional correctness
Second pass: Better ways to do the same thing (optional)
Third pass: your favorite nitpicks.
Nobody can do it perfectly, but I think trying to keep personal nitpicks out of reviews as much as possible - ideally by codifying team nitpicks in automated formatting/.prettierrc/whatever - lets people focus on things that matter: what the code does and how it does it.
In my experience, unreadable code is usually where the worst performance problems are, because that's where they are the hardest to find and fix.
You seem to be conflating readability with formatting, which is not what I mean by readability. Poor and inconsistent formatting adds some friction to reading, but it's a relatively minor part of readability.
So the first priority is getting the change to a readable state. I'm not talking about nits, I'm talking about minimizing the cognitive overhead to truly understand what the code is doing.
That being said, it is also easy to identify nits during this phase. In codebases that have collections of best practices, this often helps to ensure that the change is expressed in terms of a common vocabulary. Once this pass is done, the reviewer can usually better understand the intent of the changes.
There is a spectrum between style differences and absolutely unreadable code.
If it is absolutely unreadable, sure, send it back to be readable. But if it is merely styling issues, I'd encourage you to understand that you are not saving any time by focusing on styling before functionality.
Far more developer time is spent reading code than writing it.
If you can speed up the time required to understand a piece a code by improving the style then it's almost always worth it. For a professional software engineer, just above "absolutely unreadable" is far too low a bar to aim for.
Imagine a developer building a PR for 2 weeks, then reviews back and forth for 2 more weeks. Now 4 weeks have passed and only now the reviewer reviews the functionality - only to find that the entire implementation is wrong/could be done in a better way.
What a waste of 4 weeks of both the author and the reviewer! This could have been short-circuited very early in the process.
On my team, we default to early feedback on functionality and let the CI enforce what it can. Everything else is debatable.
1. Review functionality and then
2. Point out something about coding style.
In fact, if the functionality is good, I even approve the PR leaving a lot of code styling comments that the author can fix at their own leisure.
It's about clearly and concisely expressing ideas.
Leave styling to linters. Reviewers should be focusing on how the code is communicating.
Too often I see code that is _maybe_ correct, but the reviewer can't actually be sure of it without manually testing it, and the chances that a future reader in 4 months will have any idea what it does or why are extremely low. Good variable and function names get you 90% of the way there yet for some reason cryptic code is still all too common in the world.
To me, reviewing for readability isn't nitpicking - it's equivalent to a mechanical engineering design review pointing out that design for maintenance or design for assembly has been entirely ignored.
1) did the code use a third party library while a standard lib is sufficient? Is there a way to use native language features (e.g. list comprehension) to make the code more idiomatic to people familiar with the language?
2) are the namings make sense in the business context? Can a new hire read just the public interface and be able to guess the usage?
3) do people have to jump around, full-text-search the code base to understand what's going on? Is automatic dependencies injection being overused? How many things people need to keep in their head to be able to follow this code?
It's all readability there, and I'd argue it's not only about style or formatting.
First it gives you an idea of how well thought out the implementation was (i.e. was this a quick hack to just finish a asked for requirement) or was a best design targeted. It also helps newer developers develop a voice. Often, at the start, newer devs will take what a more senior dev says as gospel, but by striking up a conversation and, in some ways, making them defend their choices it can help build confidence and that voice to speak up when something doesn't seem right.
Second, I've found it a good way to introduce people to new approaches to accomplishing tasks. Not everyone spends their off hours studying patterns and practices and rarely is there time during a work day to do this properly so code reviews are a natural place to bring these things up as there's concrete comparisons and examples to work with. That helps spark a dev's interest to look in to the topics further.
Software engineering is a collaborative process. I don't write code for the machine; I write it for my colleagues and my future self to read. Code we write 6 months ago looks like someone else code. Readability is important unless you're convinced that it's prototype/throwaway code.
After readability then I'd look for tests. It's a theme: good tests are also meant to be read as documentation. Beside ensuring correctness, it's also the example my colleagues or future self can refer for the module usage.
when confronted, they explained to me as one would a child, that they cared deeply about code quality
Obviously `ctx` is the most readable.
these people sound useless.
Readability as it applies to code is how easy it is to read the code and understand it in relation to how complicated the problem domain is.
If it takes a low effort, then it's high readability. If it takes a high effort, then it's low readability.
Hence, I now try really hard to ignore readability until the end.
I don't know if this partly just hostile developers, different understanding of code. Ie, is code for humans to read, or for machines to execute? Or if it's dealing with feature factory developers that just want to keep adding despite how brittle the code is becoming. I presume the problem is somewhere in between, but to another extent the normalcy of the absurd where it is normal to spend 5 minutes per line of code to understand it, it is normal to havk in features and do manual regression testing, etc..
I wrote this piece a while ago focussing on using the Must/Should/Could principle to speed up the review process and put a less opinionated framework in place to help keep code review moving.
https://careerswitchtocoding.com/blog/moscow-the-best-code-r...
It’s not an original thought from me but many people hadn’t heard about it and off the back of this post I’ve had a few people get in touch to say they’ve implemented it in their team and it’s helped improve code review speed.
Tests (end-to-end and integration) are at the bottom. If there are no tests that prove that the code works, we won't really look further because we don't even know if the code implements the right thing. Then comes interfaces, then implementation, then style.
Very easy to forget to change code that isn’t highlighted in the review.
When I was on a larger team that actively participated in sprint demos was regularly a witness to this. Senior types that came along and needed to be heard - not really having a deep understanding of the application - would pick at trivial consistency issues in the UI or whatever. Such eye-rolling behaviour and hard to believe they didn't realise how transparent it was. Probably have been guilty of it myself tbh.
Things I've learned about my own professional code preferences since
1) I will not sacrifice readability for any reason. That is the hill I'm willing to die on. Reducing code is sometimes hard and that's fine that people like to challenge themselves by doing so, but not at the expense of readability. You will forget the clever trick you employed, I promise
2) Business objectives are second class to system stability. This is because the primary business objective is to be online. Failing a few transaction is bad. Failing all the transactions is much worse.
3) There is no such thing as self documenting code. There is just good nomenclature. Documents always improve someone's experience. On boarding documents for new devs will save you so much trouble.
4) I'm a huge fan of in memory caches. I hate seeing them in code. When they work, they're wonderful. But when there are issues, they're closer to demons than to bugs.
5) Data Structures. Data Structures, Data Structures, Data Structures. And then more Data Structures. Easy pre-optimisation, and these days it usually just means knowing when not to use "the default" data structure (such as ArrayList in java)
7) One day (if you're not there already), you will be on the otherside of this review. You're almost guaranteed to remember the bad times, so pay extra special attention to when the process is working well.
A simple trick I've learned is to repeat a process that worked from a previous role at a new role who's process is not working. Works more times than not.
Can you give some examples of when not to use ArrayList?
At the last couple places formatting has been handled by auto-linters; before that, it was hit or miss, but never a dominant topic of argument.
Code formatting and style discussions that don't start and end with, hey, our code formatter is broken have absolutely no place in code review. Also, keep your damn nits to yourself. If it's a nit, it's not important.
I also think that code style, fuzzing, and benchmark tests are the most important items on my checklist. It keeps the code base from becoming a jenga tower that is brittle to change. It also keeps us honest as to whether a feature is a perf hit or not. Tests are far more important than this graphic portrays them as.
You either have to fiddle until the formater outputs proper layout, or let it rest as is and never learn how to format your code, letting the code base leaking tons of probable less than ideal layout.
On the other end are fixed processes. Have as little of this as possible, automate it if you can. That means formatting, linters and shallow static analysis.
All of this is mainly in service of building trust. If you cant have some measure of success there you are truly doomed.
> does it actually work?
I feel like the foundation of any code review has to be a bug analysis to ensure that the feature works, doesn't create regressions or fail in another supported use-case.
I have to be able to trust that my teammates are doing their best to produce working components. Otherwise code review is a bandaid covering a hole in the hull of a sinking ship.
In previous jobs, I have "reviewed" code that has clearly never been run (syntax error in a script) and it makes me question my own sanity.