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

161–170 of 233 posts

Re: Improving code review time

#161

Post by RussianCow »

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.

Name one successful company that only produces beautiful, elegant, and readable code, with no nasty warts or hacks.

I'll wait.

Re: Improving code review time

#162

Post by RussianCow »

BurningFrog wrote: post #133
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.

Some problems are inherently difficult to understand, let alone solve. I'm inclined to believe that engineers at Meta solve a lot of these types of problems.

Re: Improving code review time

#164

Post by bagels »

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.

I mean, I generally think comments with this kind of attitude aren't looking for serious responses, so I normally don't respond.

That said, there are three things to consider: 1) I honestly don't care whether you think it's garbage or not. 2) I saw some amazing software there, and I saw some that wasn't amazing. 3) It's an immense business, and there is a certain amount of inherent dysfunction that comes with that many people working on anything together much as you would see at any large company.

Re: Improving code review time

#165

Post by kqr »

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…

You seem to be thinking of "hunk" and not what Facebook calls "diff" -- what Facebook means by "diff" is closer to what many people call "commit" or even "branch", i.e. a set of interrelated changes that are sort of atomic.

Reviewing individual hunks would be crazy and even Facebook knows that.

Re: Improving code review time

#166

Post by grey-area »

lopkeny12ko wrote: post #152
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/rejectin…

It encourages looking quickly at small bits of changes in isolation as opposed taking a holistic view of the entire change, and viewing them in series as 5 min little tasks you can tick off like tiktok videos.

The review tools on github already go too far toward this by removing far too much context.

Re: Improving code review time

#167

Post by s3000 »

Have I missed the feedback from the users? There should be some quotes from team members who liked the change. Their mentioning that they start being data-driven for internal tools suggests that they start treating developers like cattle and not pets.

>Driving down Time In Review would not only make people more satisfied with their code review process, it would also increase the productivity of every engineer at Meta.

This hasn't been tested. "The average Time In Review for all diffs dropped 7 percent" - they have verified that they changed the left side of the equation, the review time, but they haven't checked the outcome, the productivity. Overall it doesn't seem like they have checked if their changes have negative side effects.

Likewise

>The choice of reviewers that an author selects for a diff is very important. Diff authors want reviewers who are going to review their code well, quickly, and who are experts for the code their diff touches.

doesn't match

>A 1.5 percent increase in diffs reviewed within 24 hours and an increase in top three recommendation accuracy (how often the actual reviewer is one of the top three suggested) from below 60 percent to nearly 75 percent.

They have shown that the people they nudge are more likely to do a code review. But are they the experts who do the review well?

The 1.5 percent in reviewed diffs could also be jitter.

*edit: Meta could extend the review process. There doesn't seem to be a review process for the review team. If they don't like to review their changes, or if they cannot find suitable reviewers, how are they qualified to role out their changes to the software development team?

Post reply on HN