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.
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.
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.
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 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.
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.
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.
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.
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.
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.
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?
I mean, I generally think comments with this kind of attitude aren't looking for serious responses, so I normally don't respond.
That said, there are three things to consider:
1) I honestly don't care whether you think it's garbage or not.
2) I saw some amazing software there, and I saw some that wasn't amazing.
3) It's an immense business, and there is a certain amount of inherent dysfunction that comes with that many people working on anything together much as you would see at any large company.
There's already many articles posted to hackernews frequently at the insane internals of the mobile app, to the "meta verse", to many other things. Most of which are posted by ex-Facebook engineers. I don't need to rehash the same conversations, nor do I need to grant Facebook of all corporations any sort of courtesy.
> Name one successful company that only produces beautiful, elegant, and readable code, with no nasty warts or hacks.
Honestly, I'd genuinely like to hear some of the answers to this, or even just mentions of good codebases to explore.
That said, I don't think that there will be that many that satisfy all of the requirements listed here, because as the scope of a project grows, there will almost inevitably be some technical debt or accidental complexity, or even subjective things.
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.
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.
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.
