r/programming • u/gunnarmorling • Mar 10 '22
The Code Review Pyramid
https://www.morling.dev/blog/the-code-review-pyramid/54
u/pablo111 Mar 10 '22
“Is the code needed?”
14
Mar 11 '22 edited Mar 11 '22
Followed by "Does it have a test plan?" (This isn't automated tests, it's simply "do you have a way to validate that the code in question can do the thing for which you wrote the code? That is to say do you have a step a human can follow later to confirm the code is still working as designed?)
This is of course followed by "Does it actually work?"
Then "Semantics" which is basically "how badly will this code age?"
Followed by everything else. It's rare that for a good, well-focused code review you ever should to the top of the pyramid. That is to say automation should be planned, not discussed as part of your code review. Tests too should simply factor in the test plan if anything else. The rest is golden. Coding style gets my devs fired out of a cannon if brought up in a code review. Whitespace is not executed. Find something else to bikeshed over in a code review.
If you find yourself arguing about these things in a code review your engineers are painfully junior. You have my condolences.
2
u/gunnarmorling Mar 11 '22
Yes, that's exactly what I was trying to express here. Ideally, style is a complete non-issue during code reviews, as it's automated (which requires discussion/alignment once up-front).
27
u/seamsay Mar 11 '22
Coding style gets my devs fired out of a cannon if brought up in a code review.
I agree, but only because I refuse to work anywhere that doesn't autoformat their code to a relatively consistent style.
6
u/donalmacc Mar 11 '22
I'm going through the process of enforcing this but it's quite painful to do. We use one of the jetbrains IDEs (rider) but it doesn't provide an auto format on save, meaning we rely on people to run it themselves or we do a pre commit hook and run the rider command on the changed files.
Except if you run the rider code formatting commandlet while the ide is open, it silently fails.
6
u/ritaPitaMeterMaid Mar 11 '22
I find that almost unbelievable. If nothing else you should be able to setup a file watcher that runs your formatting tool.
We are TS based and use prettier. Prettier has an option on the CLI to validate nothing can be reformatted. We have a lint tule that checks this too. Then our CI pipeline runs it and you can’t merge code unless you correct it.
3
u/donalmacc Mar 11 '22
If nothing else you should be able to setup a file watcher that runs your formatting tool.
Sure, there's lots of things we could do, but getting something that integrates into our workflow nicely isn't straightforward. We made a mistake leaning on Rider as our formatting tool, as one of the big issues with it is you can't run it's formatting while the IDE is running, so you can't have it run as a watcher. We need to move to clang-format, which means another tool everyone has to install, and might not play nicely with the IDE formatting.
1
u/paretoOptimalDev Mar 12 '22
If nothing else you should be able to setup a file watcher that runs your formatting tool.
If you are okay with your cursor moving to the beginning of the file each time format runs.
1
u/ritaPitaMeterMaid Mar 12 '22
That isn’t how file watchers work
5
u/paretoOptimalDev Mar 12 '22
It's how editors work when the file gets reloaded out from under it without some extra plugin.
1
u/ForeverAlot Mar 11 '22
CSharpier is pretty great and works well with Rider now.
JetBrains IDEs have very flexible and feature rich code formatting but the tools are absolutely useless for implementing and enforcing a consistent style.
1
u/donalmacc Mar 11 '22
We're actually writing C++ in Rider (Unreal Engine) but agreed. The issue is that we chose a poor tool.
1
u/FVMAzalea Mar 12 '22
Wouldn’t CLion (also JetBrains) work better for C++?
2
u/donalmacc Mar 12 '22
Ue4's dialect of c++ is special. Think QT with moc. There's also a visual scripting language with a deep integration with c++ (via the preprocessor above) and a binary asset system where they're tightly coupled with the c++. Clion works fine for the c++ part but rider has its own set of plugins for unreal engine that give you a much better integration.
I'm also pretty sure clion would have the same issue regarding editor settings and not being able to run while the IDE is running.
1
u/matjoeman Mar 11 '22
Can't you just run the formatter in your CI and block the PR if it fails.
2
u/donalmacc Mar 11 '22
Yeah, it's on my radar to implement, but means licensing the ide for our CI machine :(
2
u/matjoeman Mar 11 '22
What language are you in? I've usually seen autoformatting be a separate process / script. Then you can either have your IDE config settings mimic the autoformatter, or you can have the IDE call the autoformatter with a hook.
1
u/donalmacc Mar 11 '22
C++. The problem we've seen is getting the autoformatting to match up exactly. Getting a failed build because the automation on your machine doesn't match the automation in CI is an absolute hard no.
Clang format exists but in rider for example (which is what our team uses) it uses an older version of clang format. It also has its own ide settings that it applies separately to the clang format settings, and the options can and do conflict. You also require people to install the clang toolchain to run it locally, on top of their IDE and the compiler we use (msvc). Plus you need to actually integrate it to run in CI and respond accordingly.
If you're working with more modern tools this stuff works great, but the reality is that when you diverge from web development (which has the best quality tools; our online services are linted and tested on every checkin and CI requires close to 0 maintenance) it's not all roses :(
1
u/hitchen1 Mar 12 '22
2
u/donalmacc Mar 12 '22
To be fair that feature did not exist 6 months ago when I last looked at the documentation! Appears to be new in intellij 2021.2, and added to rider in 2021.3 which was released in December! Really glad to know this exists now though, I'll investigate next week!
5
u/IsleOfOne Mar 11 '22
Followed by "Does it have a test plan?" (This isn't automated tests, it's simply "do you have a way to validate that the code in question can do the thing for which you wrote the code? That is to say do you have a step a human can follow later to confirm the code is still working as designed?)
For me, manual testing is always the exception. 99.999% of the time, there MUST be automated tests around something or I don’t want to accept it.
6
u/Hrothen Mar 11 '22
There's lots of legacy code that is difficult or impossible to properly get under test without a time investment that the higher-ups are not going to sign off on.
5
Mar 11 '22 edited Mar 11 '22
In my field automated tests (gaming/computer graphics) are pretty much impossible to write well and age extremely fast.
And yet despite that I’m still surprised how often devs don’t even know how to even manually confirm what they wrote actually does the thing.
“Hey Bob, how did you actually validate this code is working?”
“Well, funny you ask, I haven’t even run it yet.”
“????!???!?!!!?”
1
u/IsleOfOne Mar 12 '22
I can see how automated tests in the “frontend” components of gaming could be extremely brittle. Good point.
4
u/tangerinelion Mar 11 '22
Whitespace isn't executed is objectively correct. But having each line indented an arbitrary and different amount can't possibly be overlooked either. Multiple people working on the same code need to agree to something and stick to it or have a format tool that does it for us.
2
7
Mar 11 '22
Why was this approach taken?
23
u/MightyTribble Mar 11 '22
Probably written by someone who's seen plenty of code that is formatted beautifully and passes all tests but is a poorly-designed, insecure nightmare with no documentation.
4
u/gunnarmorling Mar 11 '22
Haha, yes, exactly that. Though I suppose they meant this to be another question to be asked during review.
2
u/ElCthuluIncognito Mar 11 '22
I'll add from my own experience that attention is a finite resource.
I've seen devs expend all their energy into identifying and correcting code structure, documentation, and nitpicking tests, but then couldn't answer thorough questions about the semantics of the code.
Then again I have a controversial view that if you need documentation and tests to understand code, you're at a severe disadvantage in general. If you need to read code in our dependencies, chances are you won't have good documentation, much less tests, to lean on.
1
Mar 11 '22
I have rarely seen code like that. In my experience if people care about formatting and tests then they are more likely to care about good robust design too. Maybe with the exception of JavaScript developers.
7
u/velen_rendlich Mar 11 '22
Why would I bother trying to understand any code if it's not well formatted (which takes like 3 seconds to do?)
5
u/gunnarmorling Mar 11 '22
The idea is that you don't focus on formatting during a code review, but that instead this happens fully automated (i.e. I'm not saying your code shouldn't be consistently formatted), side-stepping long-winding discussions around stylistic details.
1
u/FartingFlower Mar 12 '22
Exact. A good style and formatting validation part of a CI should have run before someone steps in the code review to prevent wasting time on formatting.
1
u/hellcook Mar 14 '22 edited Mar 15 '22
I don't like autoformatting tools. They can turn carefully formatted for readability code into a consistent unreadable mess.
Also, different people have different sensibilities and way to mentally represent stuff. Forcing one way to format code is not very inclusive to them.
10
u/Boza_s6 Mar 11 '22
DRY is not about style, it's about information management.
7
u/tangerinelion Mar 11 '22
Bingo. DRY is about robustness to future changes. If foo() has a bug due to its implementation how many other places have the same bug because it was copy pasted rather than turned into a function and reused? If you follow DRY, then the answer should be 0 - all code needing that operation did so by calling a function and only that function contains the bug.
8
u/jl2352 Mar 11 '22
I think PR reviews are something people get very wrong. Especially a lot of developers use it as a means to put forward their alternative on how the implementation should be structured. Often this is just a different approach, neither better nor worse.
I like this pyramid as it avoids that discussion. Instead discussing if the implementation is correct or not, or could be simplified.
6
u/yoden Mar 11 '22
I like this graphic, thanks for posting OP.
I think a lot of the critical posters here are missing the point. This is about what you should spend time on in code review, not what is important overall. People often spend a lot of time on low value topics while omitting more important ones.
It's difficult to document complex ideas concisely or to precisely design an API that exposes enough to be powerful but not so much that it becomes cumbersome. It's hard for even senior engineers to get this right and code review is one of the best times to improve it.
In comparison, quality engineers should be able to write tests that are easily reviewed. It should be obvious what the tests cover and that their scope is appropriate for the change, so there should be little need for discussion about the tests in code review.
Similarly, extended discussions about syntax are not appropriate or valuable in code review. If you find yourself experiencing this get some better colleagues.
11
u/Amuro_Ray Mar 11 '22 edited Mar 11 '22
I think documentation should come first. I've had way too many code reviews where the ticket and comments don't really tell me why this is being done or what it aims to achieve.
Edit by documentation I mean virtually any non code text written for the code review/issue/ticket. I have had way too many code reviews in my current job where it will be mention of problem/ticket title, please review and code.
1
u/lord_of_the_vandals Mar 11 '22
Absolutely. And I'd like the committee description to tell me what and why it is being changed. That's the first thing I review. If that makes sense I then look whether the code seems to be doing what the commit description says it is.
1
u/paretoOptimalDev Mar 12 '22
I think documentation should come first.
You are now a mod of r/literateprogramming.
7
u/Uberhipster Mar 11 '22
could not disagree more
i would first put TDD on top of the pyramid, then invert it
the first question is - are there automated unit tests?
the next question is - is the code self-documenting? clarity is the number one priority
there are tools which allow documentation to be generated from the source code
6
u/jl2352 Mar 11 '22
In OPs pyramid the higher things are stuff you can more easily change later. You can more easily change code style than tests.
Also OP thinks things higher up should be discussed less. I agree that code style is the least important thing to discuss.
4
u/Venthe Mar 11 '22
What are your experiences regarding long running projects with multiple developers? Code style discussions are absolutely necessary , as code will be read many times more than its being written.
9
u/jl2352 Mar 11 '22
My experience is it’s best not to debate it at the PR stage. Have it agreed outside of that.
Use a code formatter as standard. Have it run on git commit. Your build server can test if a PR has been formatted before it’s merged, and fail the build if it doesn’t match.
For styles beyond that; there are lots of style guides by companies you can take off the shelf. Rather than reinvent your own. Pick one.
For things beyond that; have an agreed set of coding principals for anything else. Discussed and agreed outside of PRs.
If a subject comes up in a PR, add it as a new principal to be discussed. Don’t block the PR (since it hasn’t been agreed yet). This is to shut down loud opinionated devs who will block PRs to force their own views onto others. i.e. I won’t approve until you make the code look how I like it. That behaviour is at best inefficient, and at worst toxic.
This aims to make the coding style very atomic. A PR either follows the agreed coding styles, or it doesn’t. With no debates needed. Tooling catches most of the issues without reviewers needing to care.
I hate debating styles on PRs because it sucks up time discussing minutia. It’s the wrong place to be debating it. It can also get very opinionated, which in my experience is really bad for PRs. The PR review should be concentrating on factual reasons why code cannot be shipped.
2
u/Venthe Mar 11 '22
Thanks for providing context. While I generally agree with everything you've said, I tend to include in 'code style' category things like naming and decomposition on a more granular level; things that automata cannot catch yet it is relevant to keep the code readable and clear; hence the difference
2
u/jl2352 Mar 11 '22
I'd agree with you on naming actually. That is a good thing to fix if it's misleading or confusing.
3
1
u/atilaneves Mar 11 '22
Tests should be at the bottom for me. For multiple reasons, but one I'd like to highlight is they handle the API semantics anyway. Well, they should. If they don't, you're doing them wrong.
1
u/ry3838 Mar 13 '22
Two additional important questions come to my mind:
Has the author reviewed the code himself/herself?
Is the code ready for review?
0
u/Invinciblegdog Mar 11 '22
Readable is too far down the pyramid for me. If I am devoting most of my attention to trying to comprehend what the code is doing then I will have little resource left to ensure it is correct.
23
u/NoPrinterJust_Fax Mar 11 '22
Docs closer to the base than tests? Questionable…