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

61–70 of 233 posts

Re: Improving code review time

#61

Post by PhasmaFelis »

c54 wrote: post #37
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?

I think this basically doesn’t matter and it’s fine for institutional culture to have its own ingroup jargon. Trying to standardize for some aesthetic reason wouldn’t add anything

I don't know that this particular term is a big deal, but it's a little alarming that you think standardized terminology has no more than "aesthetic" value.

Re: Improving code review time

#62

Post by kerblang »

In the spirit of tangentialism I randomly suggest: Architecture Review!

- Prevents juniors from being blown out of the ocean into startalloverland by seniors at tail end

- Focus on the most dangerous aspects of the change that can't be fixed later

- Sets the stage for more informed programming reviews later on (lower priority to me though)

Re: Improving code review time

#63

Post by joshuamorton »

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?

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.

Re: Improving code review time

#64

Post by closeparen »

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?

Whereas “merge request” and “pull request” have very literal meanings in terms of Git / distributed version control systems, Phabricator’s “diff” is VCS neutral. Diffs don’t even have to come from a VCS at all.. we have tools that spit out Phab diffs for review of changes they’re going to apply to MySQL DBs, for example.

When you use Phab with Git you normally set it to always squash merge. Each diff becomes exactly one commit on the main branch, with its commit message reformatted by Phab. The SHA that you committed before submitting your changes to Phab never appears in the authoritative repo (if only because Phab adds Reviewed-By and other metadata).

Re: Improving code review time

#65

Post by iamstupidsimple »

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?

These missing TotW's are usually about internal tools that aren't open source.

Re: Improving code review time

#66

Post by likpok »

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?

It's all the tooling: phabricator has diffs, perforce has changelists and git has both "merge" and "pull".

Tools don't agree on what to call stuff, and everyone uses different tools (and a lot of these tools are pretty old). Perforce is from '95, phabricator is from '07. Both of those predate widespread usage of git (released '05), so it's pretty reasonable that everything is different.

Re: Improving code review time

#67

Post by yrgulation »

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.

Re: Improving code review time

#68

Post by azinman2 »

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…

This is a concept I almost pursed for my PhD over a decade ago. I’m still surprised no one has done this.

The bigger picture: context is often missing for anything complicated, be it software or a new law. Yet many hands touch and retouch the underlying material over time. If you could capture _how_ something was built, and had enough insight into the larger process to sample some of the _why_, then you could both know what changed together and what potentially impacted the final decisions. This would result in (hypothetically) tremendous gains for anyone working on or joining a project that’s bigger than can fit in the mind of one person.

Re: Improving code review time

#69

Post by ok_dad »

CharlieDigital wrote: post #24
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…

I find Google's take on code reviews to be particularly good: https://cloud.google.com/architecture/devops/devops-tech-tru...

I enjoy how they state like 3 times that code reviews should be synchronous, yet "industry-standard" (aka: what people really do) is to toss it over the fence in a PR and go back and forth for several days with stylistic bullshit.

Re: Improving code review time

#70

Post by aftbit »

zadjii wrote: post #16
kweingar wrote: post #10

Earlier quoted context omitted.

I work at a company with a similar code review culture. I have about 2 blocks of meetings per day on average. I usually check my assigned code reviews in the morning and when I come back from the meetings. Once I’m done with those then I move on to my own work. If I have too much code review to the point that I can’t get my own work done, I take my name off the reviewer list and it’s reassigned. On most days I’m stil…

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

That doesn't sound like the best ratio to me, but I don't know what you are running or how good your deploy process is at catching issues. With a 4 person team, you are probably better off discussing risky/bigger changes as a whole team, and allowing 1 review for smaller, less risky changes with well defined rollback processes.
Post reply on HN