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

81–90 of 117 posts

Re: The Code Review Pyramid

#81

Post by Bjartr »

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

The best choice is whichever is more consistent with the surrounding codebase. For an outsider to the codebase, you could argue for 'context' being better, But to someone familiar to the codebase, if contexts are always in a variable 'ctxt', then 'ctxt' will instantly parse correctly without a hitch since it'll be interpreted as a single symbol.

Re: The Code Review Pyramid

#82

Post by nradov »

hsn915 wrote: post #74
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…

This used to be common sense in the few companies I worked at around 2012. People just pushed code. We occasionally held some review sessions to go over code and exchange comments / ideas about what might be wrong with this or that code and how to prevent this kind of mistake in the future. But starting from around 2016, I could never ever find a company that does not have mandetory code reviews for every single comm…

Where do we find all of those better programmers?

Re: The Code Review Pyramid

#83

Post by sanderjd »

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 should ask about the stuff you don't understand or aren't sure about. Code review is a communication tool. If you're unsure about how something works, someone else is going to be unsure about that later. It probably needs to be clarified in code or documentation.

Re: The Code Review Pyramid

#84

Post by elpakal »

tremon wrote: post #77
ryandvm wrote: post #52

Earlier quoted context omitted.

I disagree. If you have a practice that isn't effective (e.g. code reviews that aren't preventing bugs), doubling-down is almost never the correct solution. Code reviews are like unit tests; they're helpful under the right conditions. Mostly on the tricky bits or when a dev is working in an unfamiliar area. It's a common fallacy for folks to believe that if a little of something is good, then a lot of it must be bett…

Code reviews are not meant to prevent bugs. They are for making sure an individual's effort is good enough for the team to assume further maintenance of that piece of code. That almost never means that the code must be bug-free, it means that other people than the author can follow the thought processes expressed in the code. If that leads to other people finding shallow bugs, more power to them. But that doesn't mea…

> But that doesn't mean that bug prevention is (or should be) the primary goal of code reviews.

Totally agree and well said. In my experience the mentality that code review should catch bugs puts unnecessary pressure on the reviewer which leads to people not wanting to review code, because what if they missed a bug.

Re: The Code Review Pyramid

#85

Post by sanderjd »

boredtofears wrote: post #66
cillian64 wrote: post #65

Earlier quoted context omitted.

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.

Not all developers on all code, but N developers on any given piece of code, where N should be greater than 1. Why were you asked to do the review? Is N equal to 1 so there is nobody besides the author who has the expertise needed to do the review? That's a good opportunity to start the process of raising N to 2. Is it because N is equal to 2, but the other person is out on vacation? Better start working on getting N to 3 then!

Re: The Code Review Pyramid

#86

Post by patrickwalton »

sanderjd wrote: post #83
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 should ask about the stuff you don't understand or aren't sure about. Code review is a communication tool. If you're unsure about how something works, someone else is going to be unsure about that later. It probably needs to be clarified in code or documentation.

This is where I've found the most value. It's so much more effective to communicate about implementation after the developer has taken a first stab at the problem.

Re: The Code Review Pyramid

#87

Post by patrickwalton »

jamesfinlayson wrote: post #76
jonex wrote: post #62

Earlier quoted context omitted.

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

This - last year I broke something because I'd changed a query in a file completely unrelated to the ticket I was working on, and the guy who did the review treated code review as a rubber-stamp - he completely ignored the fact I'd accidentally committed a change to payment processing code in a patch to tighten up company number validation.

Even if code reviewers only made sure the files changed made sense, the reviews would be worth it.

Re: The Code Review Pyramid

#88

Post by deschutes »

I find reviews work best when you place a strong bias towards shipping workable if imperfect code. Quick reviews are also a strongly positive thing.

On the other end are fixed processes. Have as little of this as possible, automate it if you can. That means formatting, linters and shallow static analysis.

All of this is mainly in service of building trust. If you cant have some measure of success there you are truly doomed.

Re: The Code Review Pyramid

#89

Post by anonymoushn »

AnimalMuppet wrote: post #56
ryandvm wrote: post #52

Earlier quoted context omitted.

I disagree. If you have a practice that isn't effective (e.g. code reviews that aren't preventing bugs), doubling-down is almost never the correct solution. Code reviews are like unit tests; they're helpful under the right conditions. Mostly on the tricky bits or when a dev is working in an unfamiliar area. It's a common fallacy for folks to believe that if a little of something is good, then a lot of it must be bett…

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…

"a lot of organizations do X" doesn't seem like a great reason to do X. A lot of organizations use microservices.

Re: The Code Review Pyramid

#90

Post by hinkley »

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…

There's a flip between when the feedback is super, super helpful (consensus building, as you say), to a moderate inconvenience to screwing up your estimates because you seem to always post a PR right after the reviewers have started some new thing.

Then farther down that continuum someone is busting your chops for some stupid reason that, after you get back from lunch or coffee, you realize is completely valid (or even gives you an idea for an enhancement) and okay you change how a conditional works or add some more tests.

It makes me wonder if in a corporate setting PRs would be better treated as pseudorandom audits, based on a random factor and a weight for automated linting (so people don't learn to game the linter, which can be some of the worst code you ever see.)

A related issue is that most tools make it pretty difficult to look at and interact with old PRs. Feedback loops have some slop, and while it's better that you get the feedback as you are writing the code, fast follow often works as well, and I think we'd be better as a group if you could request revisions on a recently landed PR after the fact. This is mostly okay but you really need to fix your variable names, conditional block, and you missed a boundary condition over here that might not break today, but there are already stories in the backlog that will break it for sure.

Post reply on HN