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

71–80 of 233 posts

Re: Improving code review time

#71

Post by 98codes »

yrgulation wrote: post #67

Too many think code reviews are an opportunity for endless debates over personal preference. A code review should be fast and cover blatant good practice violations and architectural mistakes. Everything else should be taken care of by linters and tools. If a reviewer wants code done in a different way they can write the code themselves.

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.

Re: Improving code review time

#72

Post by haney »

Dwolb wrote: post #23
ummonk wrote: post #21

Earlier quoted context omitted.

I'd say local code quality is generally good but overall it's a big hodge podge of small features duct-taped together, so the whole app becomes a tangled mess. The metric driven culture tends to emphasize impact, not diffs landed.

How would you measure impact?

A change you made moved some metric a measurable amount (usually aligned with a team or product goal).

You put these little nuggets of impact on your performance review.

Re: Improving code review time

#73

Post by CogitoCogito »

andreygrehov wrote: post #25

Meta: > At Meta we call an individual set of changes made to the codebase a “diff.” GitHub: > Pull request Amazon: > Change Request GitLab: > Merge Request Google: > Changelist Nitpicking, but jesus christ, why can't we stick to a single term?

> Nitpicking, but jesus christ, why can't we stick to a single term?

Because different groups of people develop their own languages. This is pretty standard for humanity across all time periods, disciplines, cultures, etc.

Re: Improving code review time

#74

Post by jrockway »

joshuamorton wrote: post #63
aftbit wrote: post #47

Earlier quoted context omitted.

Oh, Abseil is new to me. Thanks! Do you know where the missing tips are? For example: https://abseil.io/tips/110 The root page (parent's [2]) mentions this one as famous by name: >Often they are cited by number, and some have become known simply as “totw/110” or “totw/77”. Is totw/110 some Google secret sauce that we are forbidden from knowing or did they skip some weeks?

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.

Re: Improving code review time

#75

Post by alecbz »

yrgulation wrote: post #67

Too many think code reviews are an opportunity for endless debates over personal preference. A code review should be fast and cover blatant good practice violations and architectural mistakes. Everything else should be taken care of by linters and tools. If a reviewer wants code done in a different way they can write the code themselves.

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 what's happening, I think it'd be clearer if we organized things like...".

Re: Improving code review time

#76

Post by gorgoiler »

andreygrehov wrote: post #25

Meta: > At Meta we call an individual set of changes made to the codebase a “diff.” GitHub: > Pull request Amazon: > Change Request GitLab: > Merge Request Google: > Changelist Nitpicking, but jesus christ, why can't we stick to a single term?

They aren’t all the same thing. A diff (or patch) is a change to the code that’s not tied to the history in any place. Linux kernel patches and Meta diffs are like this.

Pull Requests and Merge Requests are more complicated, and the unit of change is a branch. Many people reject this workflow because ultimately a branch of commits can be summed to one single diff and that’s the only thing that matters in the wider project, outside of your private branches on your personal machine.

GitLab doesn’t support anything other than merging hence the name. Most people who prefer linear history will never merge. Instead they’ll land their change on the tip of the branch, each developer taking it in turns to advance the linear history one step at a time.

I don’t know about changelists or change requests.

Re: Improving code review time

#77

Post by rayxi271828 »

andreygrehov wrote: post #25

Meta: > At Meta we call an individual set of changes made to the codebase a “diff.” GitHub: > Pull request Amazon: > Change Request GitLab: > Merge Request Google: > Changelist Nitpicking, but jesus christ, why can't we stick to a single term?

Of all these, the one that confused me the most when I encountered them was surely "pull request".

Re: Improving code review time

#78

Post by aftbit »

kentonv wrote: post #15
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…

The most important reason for code review is not to fix the code but rather to transfer knowledge between developers. I've learned a lot of techniques for writing better code from suggestions from my code reviewers, and from reviewing other people's code. Without code review, when will a junior developer ever learn anything from a senior developer?

In that case, it doesn't matter if you do the code review before or after they merge and deploy. You could read through the diffs in the commit log whenever it is convenient for you, and leave review comments for the other developer on their PR after the fact.

Re: Improving code review time

#79

Post by aftbit »

cableshaft wrote: post #22
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…

So you're advocating for pair programming all of the time? As someone who has to do both pair programming and code reviews quite a bit this past year (mentoring some quite junior developers), I find pair programming much more mentally exhausting. Two hours of that and my mind is pretty much spent for the day.

I don't read the parent as advocating for pair programming, just that any given part of the code should be understood by at least two people. This is possible to achieve even on a fully asynchronous team.

Re: Improving code review time

#80

Post by kccqzy »

aftbit wrote: post #47
sgeisenh wrote: post #9

Earlier quoted context omitted.

Google places a pretty high emphasis on code quality and readability. It's not universally great, but it's a big part of the culture. You can catch a glimpse in their [style guides][1], [abseil totws][2] and [aips][3]. Almost every change is required to be reviewed for "readability" in addition to functionality. This can feel like a lot, but it leads to pretty consistent style across the codebase which makes it a lot…

Oh, Abseil is new to me. Thanks! Do you know where the missing tips are? For example: https://abseil.io/tips/110 The root page (parent's [2]) mentions this one as famous by name: >Often they are cited by number, and some have become known simply as “totw/110” or “totw/77”. Is totw/110 some Google secret sauce that we are forbidden from knowing or did they skip some weeks?

I asked a Google friend to find totw/110 for me and apparently it's just some tips about construction and destruction of global variables.
Post reply on HN