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

31–40 of 117 posts

Re: The Code Review Pyramid

#31

Post by the_jeremy »

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…

I trust the people whose code I review to be competent developers. But everyone occasionally makes typos, misreads requirements, has blind spots, etc. Even if the creator is the expert and the reviewer is there to catch only obvious mistakes, that still has value.

Re: The Code Review Pyramid

#32

Post by pcmoney »

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…

If your team does not share a subjective understanding of what makes their product/API useful none of this matters. Focusing on objective technicalities is the least useful aspect of code review. If you have someone who constantly is like "Well that's subjective" and derails conversation that way you need to coach them or move them off the team.

Re: The Code Review Pyramid

#33

Post by Quarrelsome »

pc86 wrote: post #16
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 dat…

No, I mean like the primary function of every PR is to ensure the code does what its trying to do and doesn't (for example) fail to compile, or break the build or create an infinite loop or an out of memory exception.

Re: The Code Review Pyramid

#34

Post by bmj »

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, the level of scrutiny a merge request receives depends on the author and the functionality. MRs from a principal or lead engineer tend to get rubber-stamped at a higher rate, though, to your point, many of us will explicitly call out code that we would like to have reviewed in more detail. If a more junior level developer submits a merge request, it is highly likely their code will get a close review.

I'm not sure I agree with your premise that code reviews are the product of a culture with a lack of trust and ownership. I think, in fact, submitting to a review process is a sign of ownership -- it's saying "hey, the quality of the code we produce as a team is important to me, and I know I make mistakes."

Re: The Code Review Pyramid

#35

Post by digisign »

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…

Yes on trust. However… I'm pretty darn experienced in what I do these days, and every day I still write a bit of crappy code, usually as I come to an understanding over time of the current problem. WTFs come from code written six months ago as well as yesterday. If someone can point out an issue early, it saves everyone time. So, I welcome it.

Re: The Code Review Pyramid

#36

Post by VBprogrammer »

I've noticed a phenomenon where I sometimes have to comment about something near the pointy end of the code review pyramid in order to get my brain into the correct mindset to really start grocking how and why someone has made the changes they have. After I break this barrier it often leads to the deeper questions about whether it is doing what it supposed to be doing.

Re: The Code Review Pyramid

#37

Post by digisign »

Quarrelsome wrote: post #33
pc86 wrote: post #16

Earlier quoted context omitted.

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

No, I mean like the primary function of every PR is to ensure the code does what its trying to do and doesn't (for example) fail to compile, or break the build or create an infinite loop or an out of memory exception.

Arguably too fundamental to mention, at least in one slide. Perhaps covered during the developer-testing period, before peer review.

Re: The Code Review Pyramid

#38

Post by nescioquid »

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.

I think the purpose of code reviews is really information sharing, rather than attempting to identify and remove defects (which might occasionally happen, but is more reliably addressed via testing). On the one hand, the team has the opportunity to see how a new feature works, on the other, the implementer gets feedback on team norms.

Re: The Code Review Pyramid

#39

Post by GaveUp »

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 don't know if I agree that the second pass is optional. I've found that pass is the one I've seen have the most benefit, particularly with newer developers. It serves two purposes when done well.

First it gives you an idea of how well thought out the implementation was (i.e. was this a quick hack to just finish a asked for requirement) or was a best design targeted. It also helps newer developers develop a voice. Often, at the start, newer devs will take what a more senior dev says as gospel, but by striking up a conversation and, in some ways, making them defend their choices it can help build confidence and that voice to speak up when something doesn't seem right.

Second, I've found it a good way to introduce people to new approaches to accomplishing tasks. Not everyone spends their off hours studying patterns and practices and rarely is there time during a work day to do this properly so code reviews are a natural place to bring these things up as there's concrete comparisons and examples to work with. That helps spark a dev's interest to look in to the topics further.

Re: The Code Review Pyramid

#40

Post by owlbite »

Completely different to the order I use, which basically amounts to "how hard is it to fix this later".

So most important questions are: (1) API must be as correct as possible, especially if its exposed outside of the immediate team. Changing APIs is a royal pain. (2) Documentation of what's going on, especially for non-obvious stuff. If we come back to this in 3 years time and the author has moved on, do we have any hope of fixing it? (3) Testing. What degree of confidence do we have this is doing the right thing? Will it protect me when I change it in a few years time?

Any minor bugs or code style issues can be fixed later as/when they are found and cause problems.

Post reply on HN