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

61–70 of 117 posts

Re: The Code Review Pyramid

#61

Post by librish »

quadrifoliate wrote: post #46
willcipriano wrote: post #18

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…

The popularity this sentiment on HN terrifies me. Please don't do this. I keep finding rubber stamp code reviews (or none at all) while diving into codebases for where bugs were introduced, and 9 times out of 10, it's someone who didn't bother to get proper reviews for their code because "it's just something slowing me down". I can assure you that you have bugs and usability issues in your code, not just "linter issu…

Velocity vs correctness is a trade-off. Stringent review, testing, and ownership requirements can easily slow down developers by an order of magnitude. Which is often the right call, but not always.

Re: The Code Review Pyramid

#62

Post by jonex »

boredtofears wrote: post #59
AnimalMuppet wrote: post #56

Earlier quoted context omitted.

If your code reviews aren't catching bugs, the correct answer is to fix the code reviews , not to stop doing them. I mean, it's possible that code reviews are worthless. But enough people have found value in them, across enough organizations, enough code styles, and enough decades, that it seems really hard to conclude that. It seems more probable that the team that finds them useless is doing them wrong. (Yeah, I kn…

I don't disagree with you at all, but I'm not sure I've been a part of an organization that does it right. Beyond the stuff like style, conventions, and obvious bugs/problems, it's challenging to provide meaningful insight to someone that has many more hours of experience in a particular area of a codebase more than you do. I know I personally hold back from calling out things that I find to be subjective -- even if…

"Beyond the stuff like style, conventions, and obvious bugs/problems ..." Catching the obvious issues is IMO one of the main benefits of never skipping code reviews. Even trivial changes often have those (at least when I write them) and I've most likely saved many hours of debugging by a reviewer catching stuff like that. But this does require more than simple rubber stamping. I encourage my reviewers to read my code with the assumption that I make errors, basically looking for the bug that's fairly likely to be there.

The other thing I think is easy to do is simply see if you understand the code. If the reviewer struggle to do understand what's going on, chances are that other people will struggle as well. In this case, your lack of knowledge is an asset, the original author is likely to not see how it looks for someone not as well versed with the problem.

Re: The Code Review Pyramid

#63

Post by nine_zeros »

chimprich wrote: post #45
nine_zeros wrote: post #19

Earlier quoted context omitted.

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

> I'd encourage you to understand that you are not saving any time by focusing on styling before functionality. Far more developer time is spent reading code than writing it. If you can speed up the time required to understand a piece a code by improving the style then it's almost always worth it. For a professional software engineer, just above "absolutely unreadable" is far too low a bar to aim for.

I am still not convinced that readability comes before functionality.

Imagine a developer building a PR for 2 weeks, then reviews back and forth for 2 more weeks. Now 4 weeks have passed and only now the reviewer reviews the functionality - only to find that the entire implementation is wrong/could be done in a better way.

What a waste of 4 weeks of both the author and the reviewer! This could have been short-circuited very early in the process.

On my team, we default to early feedback on functionality and let the CI enforce what it can. Everything else is debatable.

Re: The Code Review Pyramid

#64

Post by edgyquant »

willcipriano wrote: post #18

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…

On my team we have one other engineer, no matter your or their official position, just do a quick glance over for obvious issues. This does not prevent real bugs from getting merged in, the test cases are supposedly for that but even those have some slippage.

Re: The Code Review Pyramid

#65

Post by cillian64 »

boredtofears wrote: post #59
AnimalMuppet wrote: post #56

Earlier quoted context omitted.

If your code reviews aren't catching bugs, the correct answer is to fix the code reviews , not to stop doing them. I mean, it's possible that code reviews are worthless. But enough people have found value in them, across enough organizations, enough code styles, and enough decades, that it seems really hard to conclude that. It seems more probable that the team that finds them useless is doing them wrong. (Yeah, I kn…

I don't disagree with you at all, but I'm not sure I've been a part of an organization that does it right. Beyond the stuff like style, conventions, and obvious bugs/problems, it's challenging to provide meaningful insight to someone that has many more hours of experience in a particular area of a codebase more than you do. I know I personally hold back from calling out things that I find to be subjective -- even if…

You assume here that people only work on the area of the codebase which they are most familiar with. I think a sign of a healthy team is when people are contributing to other parts of the code and not getting stuck in a rut where each area of code only has one maintainer/gatekeeper.

Also, for less experienced members, reviewing the changes of more experienced people is a great way to learn if you properly sit down and understand the change (why it’s being done, why it’s being done this way) rather than just skimming through. In addition to catching mistakes, this is useful to check that code is actually comprehensible to people other than the author and a good time to request more/better comments if necessary.

Re: The Code Review Pyramid

#66

Post by boredtofears »

cillian64 wrote: post #65
boredtofears wrote: post #59

Earlier quoted context omitted.

I don't disagree with you at all, but I'm not sure I've been a part of an organization that does it right. Beyond the stuff like style, conventions, and obvious bugs/problems, it's challenging to provide meaningful insight to someone that has many more hours of experience in a particular area of a codebase more than you do. I know I personally hold back from calling out things that I find to be subjective -- even if…

You assume here that people only work on the area of the codebase which they are most familiar with. I think a sign of a healthy team is when people are contributing to other parts of the code and not getting stuck in a rut where each area of code only has one maintainer/gatekeeper. Also, for less experienced members, reviewing the changes of more experienced people is a great way to learn if you properly sit down an…

I’m not assuming that at all. When you’re at a company that has $Xmm lines of code floating around in its repositories, it’s unrealistic to assume you will be able to cross functionally train all developers on all code. Of course you try to aim for that, but you’ll inevitably find yourself in situations where you don’t.

Re: The Code Review Pyramid

#67

Post by mpalczewski »

convolvatron wrote: post #21
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.

unfortunately, 'readability' is fairly subjective. I was part of a group recently that spent a good couple hours every week arguing whether 'ctxt' or 'context' was more readable. when confronted, they explained to me as one would a child, that they cared deeply about code quality

> when confronted, they explained to me as one would a child, that they cared deeply about code quality

these people sound useless.

Readability as it applies to code is how easy it is to read the code and understand it in relation to how complicated the problem domain is.

Re: The Code Review Pyramid

#68

Post by mpalczewski »

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.

yes, the strawman of nitpicking has been killed.

Re: The Code Review Pyramid

#69

Post by nhoughto »

I'll use this I think, very handy. I'd probably emphasis all the text/descriptions more too, lots of important nuggets in there that aren't obvious when glancing, like automating the top portions of the pyramid, humans shouldn't be mis-formatting code and others human comment on it anymore.

Re: The Code Review Pyramid

#70

Post by jeltz »

nine_zeros wrote: post #63
chimprich wrote: post #45

Earlier quoted context omitted.

> I'd encourage you to understand that you are not saving any time by focusing on styling before functionality. Far more developer time is spent reading code than writing it. If you can speed up the time required to understand a piece a code by improving the style then it's almost always worth it. For a professional software engineer, just above "absolutely unreadable" is far too low a bar to aim for.

I am still not convinced that readability comes before functionality. Imagine a developer building a PR for 2 weeks, then reviews back and forth for 2 more weeks. Now 4 weeks have passed and only now the reviewer reviews the functionality - only to find that the entire implementation is wrong/could be done in a better way. What a waste of 4 weeks of both the author and the reviewer! This could have been short-circuit…

Trying to understand the functionality of code which is hard to read is also a huge waste of time. You should try to keep the code easy to read even in very early drafts instead of leaving that until the end.
Post reply on HN