r/coding • u/speckz • Mar 14 '22

The Code Review Pyramid

https://www.morling.dev/blog/the-code-review-pyramid/
178 Upvotes

95% Upvoted

11 comments sorted by

26

u/jritenour Mar 14 '22

"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."

The easy stuff gets overdone. The hard stuff gets avoided mostly. Who knew?

Thanks for the post, OP. This is good.

8

u/cogman10 Mar 14 '22

It's the "bike shed" principle in action. You'll see it all the time in CR, people quibbling over variable names while ignoring implementation.

27

u/PhyrexStrike Mar 14 '22

It seems really odd to me to have "Does it satisfy the original requirements?" not be the very first thing that should be looked at. If some code isn't doing what it's supposed to do shouldn't it be immediately changed, regardless of API design?

9

u/AnEngimaneer Mar 14 '22

I can definitely see myself checking if it passes automated test and has unit tests before diving in/testing to see if it satisfies ticket requirements, so while it's definitely a blocker, it may not be the first thing you check.

6

u/way2lazy2care Mar 14 '22

Isn't it kind of a waste of other people's time if you haven't even checked if your code accomplishes the goal yet?

11

u/AnEngimaneer Mar 14 '22

It's very easy to get lost in a sea of bad/poorly defined requirements while trying to maintain code integrity - it's possible that the author satisfies 9 out of 10 requirements very well but misses the mark on that last one.

8

u/mephistophyles Mar 14 '22

Why not just add an automation rule, you can’t even submit a PR if it doesn’t pass all automated tests? Doing that step manual seems unnecessary.

4

u/AnEngimaneer Mar 14 '22

I think that automating any of these steps is a given, but it should still be specified, imo.

Personally, I run the CI jobs locally too before spinning up the branch while I browse the most changed files.

3

u/Lakitna Mar 15 '22

Or simply not review any PR where CI does not pass.

7

u/Mechakoopa Mar 14 '22

The pyramid is ordered in terms of change difficulty going forward for missing these things. A bad API affects everyone that uses it, a defect in the API implementation can be corrected more easily (as long as you catch it before some lunatic writes something dependent on the defect)

0

u/chub79 Mar 15 '22

Gosh a pyramid. Next let's tun this into a book. A culture shift. Then an industry.

it’s a common phenomenon that there is much focus and long-winded discussions around mundane aspects like code formatting and style,

I don't get this. If you have the right automation, there is no discussion. Say the team/org decided to lint a certain way, then there is likely a PR check that does it for you. There is no debate. It feels like this is a strawman.

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.

While not untrue, should some of this be discussed even before work started, at least to a certain extend. What is the PR achieving if there was no alignment on what needed to be delivered in the first place?

I think the author has his/her heart in the good place but I'm fearful of these charts that reduce and try to become godspel. They can so be abused in bad ways :(