Live data from Hacker News

Improving code review time

engineering.fb.com

111–120 of 233 posts

Re: Improving code review time

#111
post #75

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…

"I think it'd be clearer if we organized things like..." any such guidelines should be agreed upon beforehand, otherwise what's the expectation? That people rewrite their code to accommodate someone's needs? Reading, other than what's in whatever coding style the team uses, is subjective. Best to agree upon what the whole team prefers beforehand.

Re: Improving code review time

#112
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…

You can almost get there if you commit often, and structure your commits such that all the trivial stuff are done in their own commits; for instance, if you introduce an if, you add the if and end in one commit and do all the re-indenting of existing lines in another.

Re: Improving code review time

#113
post #86

Earlier quoted context omitted.

I think most people would be able to handle any one of those terms smoothly on day 1 You: "I'm going to send a pull request" Other engineer: "OK. BTW, we call them change requests here" You: "OK"

If only it were that simple. As we’ve seen in this thread, you’d instead likely have: Somebody else: Where I used to work we called them pull requests. Let’s have a meeting to discuss changing the terms to match industry standards. We may also want to create a committee to agree on other terminology changes too, like main instead of master.

I'm not sure if you're saying this as an argument for standardisation (ha, let's debate the spelling) or not, but these people will always find something, from frameworks to linting rules - the amount of discussion stays the same no matter how much is agreed

Re: Improving code review time

#114

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 you're working 8 hours and do at least 4 breaks per day - that's a break every 2 hours. If you do a break you might as well skim through emails and do some reviews. And remember that everything* is Pareto distributed, the review request sizes too. It's really not hard at all. [*] it's an exaggeration, but close to reality.

> If you do a break you might as well skim through emails and do some reviews.

That's not exactly a break...

Re: Improving code review time

#115
post #38

Here's another factor at Meta that can reduce code review time: Your performance review is based in part (maybe not a large part, but in part) on how many reviews you perform, and how many words you put in to those reviews. edit: In short, people are incentivized to review

Ex-Facebooker here: Number of reviews does factor into performance reviews, but mostly as a tie-breaker in calibrations when you're borderline in between bands, not as a primary metric. People who do disproportionately large number of reviews do get recognized and rewarded.

Re: Improving code review time

#117
post #97
post #36

Earlier quoted context omitted.

Meta had the best code review quality I've seen in my career.

As someone who has only experienced terrible code reviews, could you give a little summary of what you liked?

They probably had some pretty good cat meme image macros.

Re: Improving code review time

#118

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)

Architecture Review!

Looks a lot more fun than code review to boot:

https://www.youtube.com/watch?v=QfArEGCm7yM&t=57s

Re: Improving code review time

#119

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)

[deleted]
Post reply on HN