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

151–160 of 233 posts

Re: Improving code review time

#151

Post by solatic »

ep103 wrote: post #144

> Next reviewable diff Holy Fuck No. 90% of my PR review time goes into "Okay, how will this change impact parts of the system that this mid-level Dev doesn't understand yet?" and "Does this PR actually do what the ticket it is claiming to implement actually intended?" Reviewing diffs in isolation completely removes one's ability to do that. If you remove a person's ability to do that, what you've left them with is t…

> "Does this PR actually do what the ticket it is claiming to implement actually intended?"

Let me ask about your unspoken assumption: is this the PR reviewer's job? Maybe the PR seems to implement what the ticket asked for, then after merging it becomes clear that it didn't fully implement it, or the business stakeholders are unsatisfied, etc.?

Re: Improving code review time

#152

Post by lopkeny12ko »

ep103 wrote: post #144

> Next reviewable diff Holy Fuck No. 90% of my PR review time goes into "Okay, how will this change impact parts of the system that this mid-level Dev doesn't understand yet?" and "Does this PR actually do what the ticket it is claiming to implement actually intended?" Reviewing diffs in isolation completely removes one's ability to do that. If you remove a person's ability to do that, what you've left them with is t…

I don't understand this criticism. How does a "next reviewable diff" pop-up suggest that you're being forced to review a diff "in isolation"? As I understand it, nothing proposed in this article prevents you from reviewing the diff in context of the larger piece of software, as you would have always done.

This just seems like a feature to suggest another diff for you to review after you've finished accepting/rejecting the current diff. I'm already "in the zone" of code review so to speak, so this minimizes context switching. I see this is a good thing.

> The fact that they got rid of the part of the PR review process that matters

I don't understand this either. What "part of the PR review process" did they remove? The article does not claim to have eliminated any part of the review process.

Re: Improving code review time

#153

Post by charcircuit »

ep103 wrote: post #144

> Next reviewable diff Holy Fuck No. 90% of my PR review time goes into "Okay, how will this change impact parts of the system that this mid-level Dev doesn't understand yet?" and "Does this PR actually do what the ticket it is claiming to implement actually intended?" Reviewing diffs in isolation completely removes one's ability to do that. If you remove a person's ability to do that, what you've left them with is t…

>The fact that they got rid of the part of the PR review process that matters, and only saw a 1.5% improvement speaks to all sorts of problems in the process overall, not an improvement by this tool

The 1.5% improvement was from better suggestions on who would be a good person to review the diff.

The next reviewable diff feature "resulted in a 17 percent overall increase in review actions per day (such as accepting a diff, commenting, etc.) and that engineers that use this flow perform 44 percent more review actions than the average reviewer!"

Re: Improving code review time

#155

Post by _boffin_ »

milin wrote: post #142

Somewhere in the post, it's mentioned fb uses code ownership logic in the next review engine. If folks are interested, there's project called https://github.com/milin/gitown which does something similar in github leveraging code owners.

Thanks

Re: Improving code review time

#156

Post by dieselgate »

jackblemming wrote: post #148
bagels wrote: post #125

Earlier quoted context omitted.

Calling you out for lazy work. Teaching you how to use internal tools, company coding standards, suggesting better patterns, digging deep in to the context of system design and maintainability, suggesting other domain experts to tag in the reviews. I learned all sorts of Hack things that obviously I wouldn't learn outside from code reviews.

Quoted post unavailable.

Boo, please consider a review of the HN comment guidelines https://news.ycombinator.com/newsguidelines.html

Could you potentially explain what makes it "some of the most garbage software imaginable"?

edit: wording

Re: Improving code review time

#157

Post by dieselgate »

bagels wrote: post #125
nomel wrote: post #97

Earlier quoted context omitted.

As someone who has only experienced terrible code reviews, could you give a little summary of what you liked?

Calling you out for lazy work. Teaching you how to use internal tools, company coding standards, suggesting better patterns, digging deep in to the context of system design and maintainability, suggesting other domain experts to tag in the reviews. I learned all sorts of Hack things that obviously I wouldn't learn outside from code reviews.

How are small things like "you left a commented out print/log" or "use color var not hex code" handled - or that a 'company coding standards' type thing. Are those types of things potentially automated with a linter or similar?

Re: Improving code review time

#158

Post by jackblemming »

dieselgate wrote: post #156
jackblemming wrote: post #148

Earlier quoted context omitted.

Quoted post unavailable.

Boo, please consider a review of the HN comment guidelines https://news.ycombinator.com/newsguidelines.html Could you potentially explain what makes it "some of the most garbage software imaginable"? edit: wording

There's already many articles posted to hackernews frequently at the insane internals of the mobile app, to the "meta verse", to many other things. Most of which are posted by ex-Facebook engineers. I don't need to rehash the same conversations, nor do I need to grant Facebook of all corporations any sort of courtesy.

Re: Improving code review time

#159

Post by theptip »

bsaul wrote: post #7

there’s so many low hanging fruits for improving the quality of diff viewing. The worst code reviews are often the ones where code get refactored, leading to piles of delete / create lines that are just code being moved or slightly renamed. One very simple approach would be better git integration with the IDE, helping build commit that make sense, where a set of changes could easily be commented by the author as they…

JetBrains has a code review platform that lets you do diffs in your IDE. The idea is incredibly appealing but for some reason I found it not so great in practice. Something about viewing the diffs in a different UI gets me in a different frame of mind. I found it hard to get into review mode in my IDE.

There are advantages of course, being able to jump around and get the context at will.

Re: Improving code review time

#160

Post by RussianCow »

flamebreath447 wrote: post #139
zadjii wrote: post #16

Earlier quoted context omitted.

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

Do you use any special tools for this? I've heard of people "stacking" diffs in this way, but it seems like it would be clunky to review on GitHub.
Post reply on HN