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

221–230 of 233 posts

Re: Improving code review time

#221

Post by comfypotato »

shortstuffsushi wrote: post #11
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.

If I read it right, it described there being a "code review team." Whether they're responsible for reviewing all code, or just for coming up with these sorts of practices was not clear. ETA: with that said, the "reviewer recommender" to me would imply it's people who work on the same codebase.

The code review team is about the code review process I think; as opposed to being reviewers. Such would be the nature of a peak industry company.

Re: Improving code review time

#222

Post by grey-area »

nosianu wrote: post #195
pistoriusp wrote: post #189

Earlier quoted context omitted.

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

Yes I was talking about a pr (i.e. a collection of small diffs usually presented without much context). The smaller the pr the easier to review, but conversely the harder to see the big picture, and most tools don't give nearly enough context around the changes - I prefer to see the entire file.

Thanks for the links.

Re: Improving code review time

#223

Post by Arainach »

JamesBarney wrote: post #19
Scubabear68 wrote: post #12

I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely. But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit. A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will…

I've found PRs are usually a waste of time because people focus on such trivial bullshit because they're easy to find. 98% of the review comments I receive hark back to some handwavy explanation about maintainability. But never in any way that could actually cause a bug. when I run teams my rules with PRs are if it's not a bug, and it's not against the code review guidelines leave it. It's usually not worth the back…

This is exactly backwards.

One person writes the code. Many people read the code. If something is unclear or feels wrong to the reader, they get priority. Reviewers should leave all sorts of feedback; the response of the author should be to just implement the feedback unless they can clearly articulate why doing so is not a good idea. You're right that "it's not worth the back and forth", but completely wrong on who should be the one keeping their mouth shut.

Re: Improving code review time

#224

Post by atq2119 »

wahnfrieden wrote: post #187
98codes wrote: post #71

Earlier quoted context omitted.

One thing I've enjoyed where I am now is that PR comments come in two flavors. The first, actual feedback. The second, borderline pedantic issues that are prefaced with "nit: " in the comment. Nit comments are safely ignored but are there so that if the author wants to put in that change while changing some other issue, then OK.

the reviewer should make 'nit' changes directly rather than pass a mini-ticket for evaluation back to the author. why are reviewers so scared to modify prs at orgs?

That's an interesting thought. Personally, I feel messing with another person's personal branch is dangerous. They might already have some change locally, which is going to lead to surprises, and most of the people I work with unfortunately don't know Git well enough.

It's something that could be addressed on an organizational level perhaps.

The other point to consider is the whole teach a man to fish vs give them a fish thing. Teaching is better in the long run.

Re: Improving code review time

#225

Post by Dunedan »

RussianCow wrote: post #160
flamebreath447 wrote: post #139

Earlier quoted context omitted.

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.

If you work directly in the source repository that works actually really well on Github (if everybody works on its own fork instead, you're unfortunately out of luck). When opening a pull request, you just select the branch the pull request should be based on to be the previous branch in your stack. If one of those pull requests gets merged, Github will automatically rebase the stacked on onto the base branch. This way every pull requests contains exactly the changes you want it to contain, while you're still being able to stack changes by opening multiple pull requests, with each one being based on the branch of the previous one.

Re: Improving code review time

#227

Post by kevingoslar »

RussianCow wrote: post #160
flamebreath447 wrote: post #139

Earlier quoted context omitted.

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.

I use https://www.git-town.com/nested-feature-branches.html.

Re: Improving code review time

#228

Post by wahnfrieden »

atq2119 wrote: post #224
wahnfrieden wrote: post #187

Earlier quoted context omitted.

the reviewer should make 'nit' changes directly rather than pass a mini-ticket for evaluation back to the author. why are reviewers so scared to modify prs at orgs?

That's an interesting thought. Personally, I feel messing with another person's personal branch is dangerous. They might already have some change locally, which is going to lead to surprises, and most of the people I work with unfortunately don't know Git well enough. It's something that could be addressed on an organizational level perhaps. The other point to consider is the whole teach a man to fish vs give them a…

Teaching: you can still comment and edit. And your edit in the PR is reviewable by the author.

Org level: absolutely. Solvable thru tooling.

Re: Improving code review time

#229

Post by JamesBarney »

Arainach wrote: post #223
JamesBarney wrote: post #19

Earlier quoted context omitted.

I've found PRs are usually a waste of time because people focus on such trivial bullshit because they're easy to find. 98% of the review comments I receive hark back to some handwavy explanation about maintainability. But never in any way that could actually cause a bug. when I run teams my rules with PRs are if it's not a bug, and it's not against the code review guidelines leave it. It's usually not worth the back…

This is exactly backwards. One person writes the code. Many people read the code. If something is unclear or feels wrong to the reader, they get priority. Reviewers should leave all sorts of feedback; the response of the author should be to just implement the feedback unless they can clearly articulate why doing so is not a good idea. You're right that "it's not worth the back and forth", but completely wrong on who…

Usually the feedback is not "I don't understand this" or "I find this unclear" it's "I think it would be clearer if you did it this way instead of this way". Or "if you made this change it would make it more maintainable".

And if everyone followed everyone else's advice you end up with everyone writing code in their reviewers style instead of their own which doesn't seem like a gain.

Not to mention code changes have a cost, very few devs spend as much time testing and thinking about their code when making code changes. So many times you are trading more thought out, better tested code for less tested less thought out code. Because of this I've seen countless bugs introduced from code changes related to trivial PR requests.

Re: Improving code review time

#230

Post by Arainach »

JamesBarney wrote: post #229
Arainach wrote: post #223

Earlier quoted context omitted.

This is exactly backwards. One person writes the code. Many people read the code. If something is unclear or feels wrong to the reader, they get priority. Reviewers should leave all sorts of feedback; the response of the author should be to just implement the feedback unless they can clearly articulate why doing so is not a good idea. You're right that "it's not worth the back and forth", but completely wrong on who…

Usually the feedback is not "I don't understand this" or "I find this unclear" it's "I think it would be clearer if you did it this way instead of this way". Or "if you made this change it would make it more maintainable". And if everyone followed everyone else's advice you end up with everyone writing code in their reviewers style instead of their own which doesn't seem like a gain. Not to mention code changes have…

If bugs are being introduced, that means the team's testing culture is poor. If someone asks me to make a change in code review, I know that my modifications are fine because the unit tests still pass (and the integration tests, but it's rarer to have to touch those). If I forget to manually run them the build system runs them before my code is let in.

Writing in "the reviewer's style" is preferred because they are the reader. For most things that are "style" there should be explicit style guides; for the things that aren't, the reader's opinion is better every time.

If your team isn't getting useful code review feedback, that's also a team culture problem and your senior eng need to step up and lead by example.

"I don't understand this" is important feedback. At a minimum, more comments are needed, but it probably also signals that variable or method names need improvement.

My experience (15+ years, companies of all shapes and sizes from college shops to FAANG) is that the kind of engineer who feels that code review feedback isn't worth listening or is a waste of time to is detrimental to the team in both the short and long term.

Post reply on HN