The Code Review Pyramid
When it comes to code reviews, it’s a common phenomenon that there is much focus and long-winded discussions around mundane aspects like code formatting and style, whereas important aspects (does the code change do what it is supposed to do, is it performant, is it backwards-compatible for existing clients, and many others) tend to get less attention.
To raise awareness for the issue and providing some guidance on aspects to focus on, I shared a small visual on Twitter the other day, which I called the "Code Review Pyramid". Its intention is to help putting focus on those parts which matter the most during a code review (in my opinion, anyways), and also which parts could and should be automated.
As some folks asked for a permanent, referenceable location of that resource and others wanted to have a high-res printing version, I’m putting it here again:
You can also download the visual as an SVG file.
FAQ
-
Why is it a pyramid?
The lower parts of the pyramid should be the foundation of a code review and take up the most part of it.
-
Hey, that’s a triangle!
You might think so, but it’s a pyramid from the side.
-
Which tool did you use for creating the drawing?
Good tests double as documentation. I'd place designing for testing, followed by formal verification, fuzzing, mutation testing, unit testing, and finally (rather limited) integration testing, as more important than documentation.
Good function signatures and variable names also double as documentation. Think of documentation as that awful intersection crammed full of distracting signage. Should I speed up or slow down? Warning, elk sign ahead! Good product requires minimal documentation. But no amount of documentation will ever be sufficient for bad product.
Bad documentation repeats what the code says. Good documentation supplies the contains the context that is missing from the code. It sounds like you've worked with a lot of bad documentation.
I'd make a distinction between documentation intended for the developers of the project, and documentation intended for users of the project.
The first should be minimised - as you say @mcandre, good tests double as documentation. The second needs some kind of review from a human to make sure users know how to use your product. If you explain things badly here, there won't be any customers ;)
On the contrary, I always see little documentation for developers when much more would make it easier to understand the why of the code. Comments repeating the name of the variable in English is not documentation.
I had the exact thoughts as you @mcandre about putting testing under documentation but then I noticed the left side about where it is hardest to change in future and where to automate ....and then it makes sense to ensure docs are clear and concise as they are harder to evolve over time than tests are.
Great illustration Gunnar!
A lot of your bottom three tiers can be automated as well; There are static code analysis tools for complexity, performance, documentation, DRY, security, etc...
Those can assist, but they can't replace. Part of what you should be doing as a reviewer is looking at these things in terms of the larger system, and that system doesn't stop at the source code, but extends to the rest of the organization and environment.
Great breakdown - the story I'm telling myself is that implementation semantics all need tested, so having tests and documentation sections swapped might be appropriate?
Yes, that's an interesting one; both things are important of course (as all the layers), and I think it'd be equally defensible to swap the order of these two. I put documentation at a lower level than testing, as the former is a user-facing "contract" and thus can be harder to change later on, in comparison to some poorly written test. In the end, I consider the pyramid a conversation starter, if some team comes to a conscious conclusion that they'd prefer a different prioritization that's cool too.
Understood, this is definitely coming from a perspective that most of the documentation is internal...
You've done a great service in creating this content, even at the conversation starter level, so thanks for sharing!
In my experience a lot of time can be saved by reviewing code early. I agree that the API Semantics part - where you say to spend the most time in code reviews - is extremely important! I just think it's better to be reviewed before all the code is written. Eg. Design documents or preliminary PRS with the interface definition.
Good point, ideally these key aspects like API semantics have been discussed and agreed on upfront and in a code review only a quick validation is needed. The pyramid is based on my experience in open-source development, where this is not always the case; oftentimes, someone comes up with a large change or new feature without prior discussion (despite all the recommendations against doing that).
I agree with most of it, except that DRY and readability (in general - code maintainability) is not "code style" and should be further down the pyramid
Fantastic illustration.
Sometimes I think that with all the tools that help the code review process it's getting looser.
If anyone needs it in Portuguese, I forked it and translated it (https://github.com/diogoalbuquerque/morling.dev/blob/master/static/images/pt-br/pt-br_code_review_pyramid.svg).
Styling with Markdown is supported
Sign in with GitHub