The Code Review Pyramid
morling.dev
The Code Review Pyramid
1–10 of 117 posts
Re: The Code Review Pyramid
#2Ours is different.
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.
Re: The Code Review Pyramid
#3This doesn't seem correct to me. Each of these categories should bear equal weight, I won't spend less time on code style or tests to spend more time on API design. My code review checklist is a checklist. I spend as much time as necessary on each item in the checklist, no more no less.
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.
Re: The Code Review Pyramid
#4I like the attempt at making this process smoother, I guess where to place importance of different aspects relative to each other is always down to opinion and interpretation.
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.
Re: The Code Review Pyramid
#5This doesn't seem correct to me. Each of these categories should bear equal weight, I won't spend less time on code style or tests to spend more time on API design. My code review checklist is a checklist. I spend as much time as necessary on each item in the checklist, no more no less. I also think that code style, fuzzing, and benchmark tests are the most important items on my checklist. It keeps the code base from…
Code style is the first thing I eliminate from a review process. I find the most commonly used code formatter and linter for $LANG (the more opinionated the better) and plug it into pre-commits and CI. Code style is just not worth my time.
Re: The Code Review Pyramid
#6While I recognize the problem as real and significant, and I think a hierarchy of concerns is a valid way to mitigate it, I feel there are a few problems in this particular pyramid.
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.
Re: The Code Review Pyramid
#7This doesn't seem correct to me. Each of these categories should bear equal weight, I won't spend less time on code style or tests to spend more time on API design. My code review checklist is a checklist. I spend as much time as necessary on each item in the checklist, no more no less. I also think that code style, fuzzing, and benchmark tests are the most important items on my checklist. It keeps the code base from…
Code style is the first thing I eliminate from a review process. I find the most commonly used code formatter and linter for $LANG (the more opinionated the better) and plug it into pre-commits and CI. Code style is just not worth my time.
I’m at the point now where I don’t want to think about layout and formatting of my code, just let a formatter do it all for me on save please
Re: The Code Review Pyramid
#8First thing I review is readability.
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.
Re: The Code Review Pyramid
#9First thing I review is readability. 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.
If the code is doing the wrong thing in the first place, no amount of nitpicking on readability will save time.
First pass: Functional correctness
Second pass: Better ways to do the same thing (optional)
Third pass: your favorite nitpicks.
Re: The Code Review Pyramid
#10aren't we missing something at the base of the pyramid:
> 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.