The Code Review Pyramid - Gunnar Morling #293
giscus[bot]
Bot
Replies: 21 comments 15 replies
|
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. |
|
Documentation for developers does have use although it does depend on the type of project and the size of your code base imo. Good documentation can provide much needed context for more complex projects in large code bases especially when you have cross-cutting teams. If some other teams had even just short quick documentation in the class/method docs to provide context for crucial parts of their code/classes (why/purpose, not the how), it would save so much time. For some straightforward CRUD API project, sure developer documentation may not be needed. |
|
This. One of my pet peeves is that many tests seem to be written with only the idea that it provides some evidence that the code behaves in a certain way. Readability can be close to zero; at worst we have these 20-line setup parts immediately continuing with code that exercises the system, making it really difficult for readers to see even what is necessary setup and what is code that actually exercises desired behaviour. So nowadays I try to think of code as executable, formal specification. It's executable for two reasons: so that you can verify whether the code seems to adhere to the spec, and also because it's unambiguous. It's easy to be accidentally vague with natural language, not so with code. This doesn't mean you need to use BDD frameworks or anything like that; just think of it as a spec you write for other people, not for the compiler/interpreter. Prefer literals whenever possible, use variables only to tie two values that mean the same thing together. Extract details to private helpers, only using those values as parameters that are relevant for that particular test. This way tests code is in high level, reads almost like prose. Use long, descriptive names for test functions like |
|
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! |
|
LGTM
|
|
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). |
|
Yeah, typically you should have a design document that captures greater context/motivation/reasoning behind these fundamental behavior changes or APIs - and that is where you most productively flesh that out. PRs are not the best medium to have those discussions Models that work in some popular open-source projects, like Apache Kafka for example, are to have KIPs (Kafka Improvement Proposals) be posted whenever there's a public API or behavior change (amongst others) - this has to go through extensive discussions and voting in the mailing list before you ever get to implementation. This nicely works such that when you get to implementing, there isn't much back-and-forth in the review about these foundational things. It is a fair bit unproductive to have to re-implement foundations anyway |
|
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. |
|
completely agree with you! I usually start the code review from the bottom of the pyramid. most of nits like typo or grammar things are fixed during reflecting feedback in my experience I translated this into Korean :) hope it helps Korean engineers : https://jiyeonseo.github.io/2022/04/03/the-code-review-pyramid/ |
|
nice and useful illustration 👍 |
|
nice work! |
|
Readability should be the base of the pyramid. |
|
Thank you for the great and concise guide. I want to mention that it'll be helpful to consider the "people" aspect. (It's commented by one of my facebook friend)
Also, I'm wondering if you have any tips to better recognize the things you should have changed but didn't. It's �relatively easy to leave a comment about changes made in a commit or PR, but it's quite difficult to leave a comment about what should've changed but didn't. It's more difficult if I'm not very familiar with the codebase. I can think of a few ways, but I wish there were more tools, so I wonder how you've been recognizing.
|
|
Very Well explained. It covers most of the areas of interest while doing the code reviews. Thanks for the post. |
|
Thank you! |
|
I like and am sharing the pyramid with 1 significant tweak. For the kind of work we do, I strongly advocate top-down software testing. So for us the applicable "Tests" clause is, "...-Is it using Integration Tests where possible, subsystem and unit tests where necessary...." Any comment on this? |
|
You should definitely adapt the pyramid as it makes sense for your specific situation and context. The reason for the original way of phrasing the testing point is that I consider integration tests as a somewhat sparse resource, which you can employ only so much. Otherwise, overall execution times may become a problem. But they do have their advantages for sure, so duration of execution isn't a problem for you, adjusting this makes sense. The main idea is to have a shared understanding of the priorities within one organization. |
|
thanks - harmony is restored |
|
Helpful content thank you ! |
|
Thanks, very happy to hear that! |
|
Isn't it too late to check and discuss API Semantics, Implementation Semantics, Documentation on code review phase? From my point of view, if a dev who is working on a task feels that something needs to be discussed (security, complexity etc.), it's better to call and discuss it with other devs instead of wait until code review phase |
|
yes, absolutely agreed, there should be a discussion about design etc. upfront, and it's what I'm always recommending in the OSS projects I'm involved with. Still, not everyone does that (there is the occassional 3K lines PR coming out of the blue), and even if it was discussed, it's vital to make sure during review that things have been done the way they should have been done. Ideally, that's a quick check and one can move one. |
|
Thanks for the Dos. Now few Don'ts
|
|
This is great! Instead of "API Semantics" I would love to see it more broadly speak to "Architecture Semantics" or maybe "Architecture" could be a layer below API where we review for things like this: http://dl.apparentsoft.com/Software-architecture-tips.pdf |
|
One worth printing out for the desk area. |
|
On pyramid/triangle explanations: we all know from test automation that it's just an inverted ice cream cone. :) |
and others
Uh oh!
There was an error while loading. Please reload this page.
The Code Review Pyramid - Gunnar Morling
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 "
https://www.morling.dev/blog/the-code-review-pyramid/
All reactions