Hacker News Reader

Comment by comfypotato | Hacker News Reader

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.

View on HN

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 still able to get a good 2-3 hours of sustained focused work on my own project.

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

Also with a team so small you can't afford wasting it on meetings.

You might need focus, i.e. non-meeting days, 3 days a week.

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.

This is one of the features of (yes), Meta's sapling VCS. https://sapling-scm.com/docs/introduction/getting-started/

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.

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

So every change needs to be seen by 3 out of 5 people (assuming the lead also does code review)? That sounds a bit inefficient.

I think your main issue is having "two blocks of meetings" a day as a developer. That sounds absolutely nuts to me. Even two meetings a day is likely too much for a dev, let alone two blocks of meetings! I'd start by reducing that.

> I have about 2 blocks of meetings per day on average

Are you sure you are a developer?

If the average change is small, it's not a particularly challenging thing to take 5 minutes to review something. (for context, my median change, both mailed and reviewed, is under 50 LoC, though this will depend on language, e.g. Java is notably more boilerplate-y than the languages I usually use, and I do a lot of configuration changes that are 1-3 lines by nature).

Picking up a 5 minute review after you've returned from some existing context switch (a meeting, coffee, conversation, lunch, etc.) is easy enough.

As a result, around 25% of my reviews are done within an hour, and 10% within 10 minutes. I'm a slightly fast reviewer, but approximately the same applies to reviews done of my code. And that includes reviews that require interaction with peers in Europe, which obviously require much longer because they can be mailed while I'm asleep or the reverse.

Yeah if it's a change to code you're deeply familiar with, that's not much of a context switch. In that situation, working on your own stuff or reviewing someone else's code feels basically the same, unless one of you is doing something very wrong.

10 minutes of work to review is fascinating to me. I’m somewhat of a baby programmer, and that’s over my head a bit I think.

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.

Sure, I do reviews first thing every morning, and sometimes right after lunch (mostly just rereviews), but that would give a mean/median response time of 4 hours assuming work completion time is uniformly distributed. And if changes are requested, that would add another 2-4 hours, which brings the total review-in-wait time to a full day, which the post was saying was unacceptable. To get the numbers they claim they must be reviewing more frequently than that.

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.

Every team member doesn't do their review at the same time. You're likely to get a review within a few hours, which means total review time (they're only counting the time it's waiting for a review) is unlikely to be a whole day.

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

It's perfectly consistent with a few hours, given that different team members tend to review diffs at different time during the day.

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

Well, after or before the break. The point is you're going to lose context completely anyhow.

And I'd like to stress the distribution of the "complexity of a review", which you can crudely approximate with something like number of lines changed. Most of them will be small.

When I'm on a break, I'm pretty much always mulling my task on the back of my mind, and doing things that don't require concentration, so when I get back from the break I'm still largely in the same frame of mind. Doing code reviews definitely forces a sharper shift of mind and takes longer to switch back from than a break does. Sometimes it is worthwhile, but it does have a cost.

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.

Yes good meme macros too. I left a few in my wake.

Calling you out for lazy work. Teaching you how to use internal tools, company coding standards, suggesting better patterns, digging deep in to the context of system design and maintainability, suggesting other domain experts to tag in the reviews. I learned all sorts of Hack things that obviously I wouldn't learn outside from code reviews.

How are small things like "you left a commented out print/log" or "use color var not hex code" handled - or that a 'company coding standards' type thing. Are those types of things potentially automated with a linter or similar?

Most reviews can be done quite quickly. Small changes, test plan is convincing, low potential to break things. Besides, if engineers are good, code is easier to review.

Slow reviews means that you need to context switch a lot to answer their review suggestions and resubmit the review, that is much worse for productivity than taking a few minutes to go and look through what a teammate is doing now and then.

Alternatively your code review culture doesn't give a lot of suggestions for changes before submission, and that leads to more technical debt which will produce bad results long term.

Having thousands of engineers tends to help on that. If there's a 5% chance one engineer is available in the next two hours after you post a PR, with a team of 10 you will probably have none, with a team of 1000 you will easily get some.

Pairing can sidestep both of these problems. No context interruption and reviewed continuously.

I have found that when you pair it's easy for both people to lose sight of some greater context. You become one big mind and become susceptible to as many blindspots as an individual.

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.

Nah, a dedicated "code review team" is not a thing.

Often the suggestion featured does suggest a whole team as a review based on code ownership.

rb2k

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.

People don't like when I say this, but I think it's true:

If it typically takes you hours to "get inside" some code, that code is way too complicated.

Some problems are inherently difficult to understand, let alone solve. I'm inclined to believe that engineers at Meta solve a lot of these types of problems.

You can solve difficult problems with simple, well factored code.

Just like your can solve simple problems with spaghetti code.

Of course, writing simple, will factored code is often hard!

Reply on news.ycombinator.com