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

191–200 of 233 posts

Re: Improving code review time

#191

Post by __alexs »

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…

They measure the "quality" of review based on time spent looking at the code which suggests to me they have absolutely no idea why they are making people do code review at all.

This is an especially bad metric because

1) We have good data that after about 60 minutes of reviewing we start to lose the ability to find more issues.

2) It incentives making bigger and harder to review changes so that people spend more time looking at the code.

Re: Improving code review time

#192

Post by [deleted] »

gtirloni wrote: post #183
charcircuit wrote: post #174

Earlier quoted context omitted.

>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? I think there's assumption that people won't just rubberstamp significant diffs to code they don't own. When submitting a change to another team's project if the reviewers that are suggested aren't actually the right person they are more likely to know the right person who should review it…

> I think there's assumption that people won't just rubberstamp significant diffs to code they don't own I wouldn't advise doing that but to play devil's advocate, why shouldn't they do that when number of reviews are probably a metric in their performance review? What's in place to discourage that?

[deleted]

Re: Improving code review time

#193

Post by yuhc »

pistoriusp wrote: post #189
grey-area wrote: post #166

Earlier quoted context omitted.

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.

A diff at meta is a pull-request.

No. A diff is a commit.

Re: Improving code review time

#194

Post by comfypotato »

ummonk wrote: post #17
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.

Working there I tended to conduct code reviews each afternoon at the end of work. Sometimes after coming back from lunch as well. There isn't any "drop what they're doing" involved.

Which is what I expect. This doesn’t really agree with “a few hours” which is why I was confused.

Re: Improving code review time

#195

Post by nosianu »

pistoriusp wrote: post #189
grey-area wrote: post #166

Earlier quoted context omitted.

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.

A diff at meta is a pull-request.

As far as I can see that - and many of the other responses to the criticism - does not counter said criticism.

If you look at only the PR, even with a few lines of context, you still don't see much.

I actually like the diff view window provided by JetBrains editors through the alreedy bundled "Github plugin". I get to see the whole file, before and after (left/right), with highlights for changes, lines added, lins removed. That way I see the entire context.

If you only see the usual change+3 lines of context you often don't even see what function is impacted, and it's also rare to have the context of the entire module being changed in ones head already.

For evaluating PRs, I use the PR review feature in IDEA editors and go through the list of files changed, opening a new window with the entirety of that file available to me, changes highlighted. F7 jumps to the next change, but mostly I just scroll.

You can add review comments right in the diff view.

https://youtu.be/MoXxF3aWW8k ("IntelliJ IDEA. GitHub Pull Requests") -- Diff viewer shown at 2:40; In that video the diff view is shown inside the editor, but I much prefer - and fortunately that is configurable - to open the Diff View in a new window, maximized.

https://www.jetbrains.com/help/idea/comparing-files-and-fold...

The example works only for PRs on GitHub and IDEA editors, but I wanted to present the concept, and show that IMO very nice diff view and how it makes it easy to review changes with the context of the entire file.

Re: Improving code review time

#196

Post by Thorrez »

jrockway wrote: post #74
joshuamorton wrote: post #63

Earlier quoted context omitted.

A lot of them are related to Google-internal practices or libraries, or are like too opinionated (Google has strong internal C++ opinions that aren't necessarily correct or even reasonable elsewhere) to be useful. 110 in particular probably could be public, but it looks like they stopped externalizing them in late 2020, which is kind of sad, so I assume they never got around to it.

Yeah, that sounds about right to me. I used to help edit Testing on the Toilet and there were a lot of internal-tool-specific ones that we never made public. When we were low on content, we would happily publish an issue about someone's internal service/project, for example. It's not that we were trying to hide something, it would just be completely useless to the outside world.

TIL TotT is externalized.

https://testing.googleblog.com/

Re: Improving code review time

#197

Post by disgruntledphd2 »

gtirloni wrote: post #179
disgruntledphd2 wrote: post #177

Earlier quoted context omitted.

You can use timezones to your advantage here. When California is finishing, Singapore is starting and when Singapore is finishing London is starting.

Why would you do that unless it's a hotfix or something really critical? Are the costs negligible?

So, it depends. When I was at FB, we had one person in Europe (me), one in SF and one in Asia.

Organising the diffs in this way made it much, much easier to collaborate.

Given how much FB has grown since then, I suspect this would be even easier, and this is how the numbers noted above are what they are.

Re: Improving code review time

#198

Post by ckdot »

thehm wrote: post #181
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…

Those architectural decisions should be reviewed during the design and planning phase so mid/low-level devs don't waste time building the wrong thing in the first place.

In theory that’s right what you say.

But still, tasks should be small enough that it hopefully won’t hurt too much if you throw away all the code again.

Too often I experience that - even if you talk to low/mid level devs about a feature before, even if you make a task breakdown together with them and write all the software design decisions down, even if you tell them they should commit often to you can check once in a while, even then in the end it‘s too often garbage what has been produced. Still, companies want to keep these developers because it’s hard to find new ones. And I guess it’s our senior’s duty - even if there are many disappointments - to still assume the best and try to teach them to do better. Again and again and again.

Re: Improving code review time

#199

Post by zeckalpha »

grey-area wrote: post #166
lopkeny12ko wrote: post #152

Earlier quoted context omitted.

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.

Meta deploys small changes continuously. If you have a bigger change, it should get broken up into smaller easily reviewable atomic changes.

Re: Improving code review time

#200

Post by zeckalpha »

yuhc wrote: post #193
pistoriusp wrote: post #189

Earlier quoted context omitted.

A diff at meta is a pull-request.

No. A diff is a commit.

They might get squashed for review or before merge, making the distinction less important. When you are running a monorepo you generally don't get long series of small commits in a batch but rather a singular atomic commit.
Post reply on HN