Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

The Code Review Pyramid

morling.dev

Post reply on HN
Print view View original

11–20 of 117 posts

Re: The Code Review Pyramid

#11

Post by tuyiown »

simonbarker87 wrote: post #7
kolanos wrote: post #5

Earlier quoted context omitted.

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

But, code formatting is not a solved problem, right ?

You either have to fiddle until the formater outputs proper layout, or let it rest as is and never learn how to format your code, letting the code base leaking tons of probable less than ideal layout.

Re: The Code Review Pyramid

#12

Post by pc86 »

nine_zeros wrote: post #9
mpalczewski wrote: post #8

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

This structure (especially the optionality of the second pass) is how you end up with perfectly readable code iterating over List when a hash map would suffice and be orders of magnitude more performant.

Nobody can do it perfectly, but I think trying to keep personal nitpicks out of reviews as much as possible - ideally by codifying team nitpicks in automated formatting/.prettierrc/whatever - lets people focus on things that matter: what the code does and how it does it.

Re: The Code Review Pyramid

#13

Post by thanhhaimai »

mpalczewski wrote: post #8

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

+1, I also focus first on readability while reviewing code.

Software engineering is a collaborative process. I don't write code for the machine; I write it for my colleagues and my future self to read. Code we write 6 months ago looks like someone else code. Readability is important unless you're convinced that it's prototype/throwaway code.

After readability then I'd look for tests. It's a theme: good tests are also meant to be read as documentation. Beside ensuring correctness, it's also the example my colleagues or future self can refer for the module usage.

Re: The Code Review Pyramid

#14

Post by sgeisenh »

nine_zeros wrote: post #9
mpalczewski wrote: post #8

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

I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code.

So the first priority is getting the change to a readable state. I'm not talking about nits, I'm talking about minimizing the cognitive overhead to truly understand what the code is doing.

That being said, it is also easy to identify nits during this phase. In codebases that have collections of best practices, this often helps to ensure that the change is expressed in terms of a common vocabulary. Once this pass is done, the reviewer can usually better understand the intent of the changes.

Re: The Code Review Pyramid

#15

Post by th3iedkid »

mannykannot wrote: post #6

While 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…

Another aspect, the pyramid makes code review look like the only thing around the actual value outcome. There’s a lot of other aspects like pair-programming that eliminates most things in the pyramid, taking it out of scope from a review process

Re: The Code Review Pyramid

#16

Post by pc86 »

Quarrelsome wrote: post #10

aren'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.

Potentially unpopular opinion, but I would much rather get well-formatted, performant, readable code that misses the business case a bit than get a ball of spaghetti mess that nails it. Not only will it be easier to update the code in the former, but the business pressure to just deploy the latter and "fix it later" can sometimes be too high to resist. I was just looking at a "proof of concept" with a 2017 commit date in production last week, the danger of that is all too real.

Re: The Code Review Pyramid

#17

Post by mdtusz »

nine_zeros wrote: post #9
mpalczewski wrote: post #8

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

As a counter argument, it's much easier for a developer to write code that does the right thing when that code is readable.

Too often I see code that is _maybe_ correct, but the reviewer can't actually be sure of it without manually testing it, and the chances that a future reader in 4 months will have any idea what it does or why are extremely low. Good variable and function names get you 90% of the way there yet for some reason cryptic code is still all too common in the world.

To me, reviewing for readability isn't nitpicking - it's equivalent to a mechanical engineering design review pointing out that design for maintenance or design for assembly has been entirely ignored.

Re: The Code Review Pyramid

#18

Post by willcipriano »

The more experienced I get the more I want to work on teams with a higher degree of trust than ones like this. Don't get me wrong I think code review can be valuable but on high preforming teams it works best on a as needed basis.

The way this has worked successfully for me in the past is as a new joiner to a team you submit for code review every time. My first couple of reviews tend to have a good bit of feedback as I settle in to the style of the team and often a new language or framework. After a while though I become a domain expert for the projects I am working on, and code reviews end up becoming a rubber stamp. At that point it's just something slowing me down. I'd request review when I need it, or I'm making a big change that will effect everyone, otherwise there are many cases where there is really only one way to skin this cat and it's totally perfunctory.

This sort of arrangement also requires a bit of ownership, if you see reports floating around about an endpoint you just worked on having issues you have to be able to be trusted to jump on it. I feel like trust and ownership are touchy feely people problems so tech people invented code reviews and linters to try to make up for not cultivating the sort of culture where they aren't required on every single little thing.

Re: The Code Review Pyramid

#19

Post by nine_zeros »

sgeisenh wrote: post #14
nine_zeros wrote: post #9

Earlier quoted context omitted.

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.

I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code. So the first priority is getting the change to a readable state. I'm not talking about nits, I'm talking about minimizing the cognitive overhead to truly understand what the code is doing. That being said, it is also easy to identify nits during this phase. In codebases that have collections of best practice…

> I think you're missing the point: it's incredibly hard to determine functional correctness of unreadable code.

There is a spectrum between style differences and absolutely unreadable code.

If it is absolutely unreadable, sure, send it back to be readable. But if it is merely styling issues, I'd encourage you to understand that you are not saving any time by focusing on styling before functionality.

Re: The Code Review Pyramid

#20

Post by thanhhaimai »

nine_zeros wrote: post #9
mpalczewski wrote: post #8

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

It's assumed in your post that readability review is nitpicking. It's actually about making the code idiomatic, consistent, predictable, quickly understandable, and low mental stress while reading. The "nitpicking" parts you referred mostly is about ensuring code consistency and predictability. But there are more to readability than that:

1) did the code use a third party library while a standard lib is sufficient? Is there a way to use native language features (e.g. list comprehension) to make the code more idiomatic to people familiar with the language?

2) are the namings make sense in the business context? Can a new hire read just the public interface and be able to guess the usage?

3) do people have to jump around, full-text-search the code base to understand what's going on? Is automatic dependencies injection being overused? How many things people need to keep in their head to be able to follow this code?

It's all readability there, and I'd argue it's not only about style or formatting.

Post reply on HN