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

Improving code review time

engineering.fb.com

Post reply on HN
Print view View original

131–140 of 233 posts

Re: Improving code review time

#131

Post by azinman2 »

lamontcg wrote: post #104
azinman2 wrote: post #68

Earlier quoted context omitted.

This is a concept I almost pursed for my PhD over a decade ago. I’m still surprised no one has done this. The bigger picture: context is often missing for anything complicated, be it software or a new law. Yet many hands touch and retouch the underlying material over time. If you could capture _how_ something was built, and had enough insight into the larger process to sample some of the _why_, then you could both kn…

The PR history can answer the why, and if you can anticipate someone will ask why because you know it is edge-condition spaghetti then you can document it right in a comment.

This is a pretty myopic stance that says everything is fine. It’s not. PR histories can be gigantic, interwoven, doesn’t tell you how and only sometimes tells you the why—-usually at a very fine grain of detail. Saying that you can “document it right” is like saying “code it well in the first place.” Much of the time there aren’t great comments or PRs, and what there is ends up assuming a huge amount of knowledge about the why/how. If you’re an outsider coming in, we should be able to do way better to help.

Re: Improving code review time

#132

Post by itsdrewmiller »

davidmurdoch wrote: post #121

All these comments about how code review is a waste of time, or suggest code review is only for bugs, really shine light on why so much software is incredibly slow today.

None of them engage with the content of the article either - having a "play next" button for code review is awesome assuming it works reasonably well. I'm curious about the quality of nudgebot reviews. In my experience the PRs that sit around forever are the 3000 line epic find/replace refactors all done in one commit that are impossible to really review. I skimmed the paper and didn't see any accounting for "diff time to diff length", so I'm not sure the result there is anything meaningful. Maybe people are getting faster feedback to not submit such shitty PRs.

Re: Improving code review time

#133

Post by BurningFrog »

comfypotato wrote: post #5

Am I reading this right? If the total median time in review is “a few hours”, that means that people are dropping what they’re doing to review the code. That can’t really be a good code review? A context switch alone would be half of that time for me.

People don't like when I say this, but I think it's true:

If it typically takes you hours to "get inside" some code, that code is way too complicated.

Re: Improving code review time

#134

Post by BurningFrog »

proc0 wrote: post #43

If you're going to add machines to the process why not add it with the purpose of eliminating the human from the process all together? Reviews are necessary because compilers and linters can't catch everything. Runtime bugs that are not caught by the pipeline tend to be edge cases that don't happen until there is enough data to test (in the general sense) the feature. ML could be used for smart testing and if it pass…

You can never automate how readable the code is or how good the design is.

Re: Improving code review time

#135

Post by pavon »

H8crilA wrote: post #128
screamingninja wrote: post #114

Earlier quoted context omitted.

> If you do a break you might as well skim through emails and do some reviews. That's not exactly a break...

Well, after or before the break. The point is you're going to lose context completely anyhow. And I'd like to stress the distribution of the "complexity of a review", which you can crudely approximate with something like number of lines changed. Most of them will be small.

When I'm on a break, I'm pretty much always mulling my task on the back of my mind, and doing things that don't require concentration, so when I get back from the break I'm still largely in the same frame of mind. Doing code reviews definitely forces a sharper shift of mind and takes longer to switch back from than a break does. Sometimes it is worthwhile, but it does have a cost.

Re: Improving code review time

#136

Post by Jensson »

yrgulation wrote: post #111
alecbz wrote: post #75

Earlier quoted context omitted.

I agree that anything that _could_ be covered by an automated check ought to be. But don't think I'm convinced that everything else is either an egregious mistake or isn't worth discussing. IMO a big part of what you ought to be reviewing for is readability, which does sometimes overlap with personal preference. But there's a spectrum from "I'd indent these columns a little differently" to "it's hard for me to follow…

"I think it'd be clearer if we organized things like..." any such guidelines should be agreed upon beforehand, otherwise what's the expectation? That people rewrite their code to accommodate someone's needs? Reading, other than what's in whatever coding style the team uses, is subjective. Best to agree upon what the whole team prefers beforehand.

The expectation is that you talk with the person who did the review and discuss the best place to put things. Those discussions are good for code health, and it is much easier to have them when you have new code in front of people so they can see what it looks like.

If you can't have those discussions in a code review then your code review process is too slow, and you ought to fix it so that it is easy to have these discussions there.

Re: Improving code review time

#137

Post by Jensson »

ok_dad wrote: post #69
CharlieDigital wrote: post #24

Earlier quoted context omitted.

I find Google's take on code reviews to be particularly good: https://cloud.google.com/architecture/devops/devops-tech-tru...

I enjoy how they state like 3 times that code reviews should be synchronous, yet "industry-standard" (aka: what people really do) is to toss it over the fence in a PR and go back and forth for several days with stylistic bullshit.

Code reviews are usually synchronous at Google though, commenting and fixing things is like chatting with the reviewer so are usually done quickly. Not sure why this wouldn't be industry standard, is there any reason to make code reviews more painful than that?

Re: Improving code review time

#138

Post by riwsky »

itsdrewmiller wrote: post #132
davidmurdoch wrote: post #121

All these comments about how code review is a waste of time, or suggest code review is only for bugs, really shine light on why so much software is incredibly slow today.

None of them engage with the content of the article either - having a "play next" button for code review is awesome assuming it works reasonably well. I'm curious about the quality of nudgebot reviews. In my experience the PRs that sit around forever are the 3000 line epic find/replace refactors all done in one commit that are impossible to really review. I skimmed the paper and didn't see any accounting for "diff ti…

what stops you from reviewing the sed one-liner that created the diff, instead?

Re: Improving code review time

#139

Post by flamebreath447 »

zadjii wrote: post #16
kweingar wrote: post #10

Earlier quoted context omitted.

I work at a company with a similar code review culture. I have about 2 blocks of meetings per day on average. I usually check my assigned code reviews in the morning and when I come back from the meetings. Once I’m done with those then I move on to my own work. If I have too much code review to the point that I can’t get my own work done, I take my name off the reviewer list and it’s reassigned. On most days I’m stil…

> I take my name off the reviewer list and it’s reassigned. How big is your team? We've got 4 developers on our team (and one lead), and require two signoffs for code changes. There's just not enough of us for something like that to work... right?

I work on similar sized teams, I introduce my PRs as stacked so they are extremely consumeable in very small bite-sized code changes and build on the last one reviewed.

Re: Improving code review time

#140

Post by jeffbee »

kerblang wrote: post #62

In the spirit of tangentialism I randomly suggest: Architecture Review! - Prevents juniors from being blown out of the ocean into startalloverland by seniors at tail end - Focus on the most dangerous aspects of the change that can't be fixed later - Sets the stage for more informed programming reviews later on (lower priority to me though)

I've seen mixed things from architecture reviews. I've seen it used by people with titles that exceeded their actual abilities, to stop people with junior titles from doing things the senior person simply didn't understand. And I've seen architecture reviews used just to satisfy the whims of senior people, to gratify that urge to nitpick or dictate what language they wanted to use. Those are the bad ways.

The good way I've seen architecture review used is nobody was going to tell you not to write or even deploy whatever the hell it was that you thought you wanted to write, but if you wanted to integrate with Grown Up Systems, there were ACLs that your system would not be added to unless and until your system had passed the review of the Grown Ups. I think this way is strictly better for two reasons: it can't strangle good ideas at birth, and it minimizes the amount of architecture reviewing that everyone needs to do, because half of the junk that gets brought to pre-implementation arch reviews never gets built anyway.

Post reply on HN