7 ms·
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 ca
by comfypotato 4y ago
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.
- ummonk 4y agoWorking 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.
- pavon 4y agoSure, 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.
- disgruntledphd2 4y agoYou can use timezones to your advantage here. When California is finishing, Singapore is starting and when Singapore is finishing London is starting.
- gtirloni 4y agoWhy would you do that unless it's a hotfix or something really critical? Are the costs negligible?
- disgruntledphd2 4y agoSo, 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.
- ummonk 4y agoEvery 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.
- comfypotato 4y agoWhich is what I expect. This doesn’t really agree with “a few hours” which is why I was confused.
- ummonk 4y agoIt's perfectly consistent with a few hours, given that different team members tend to review diffs at different time during the day.
- kweingar 4y agoI 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.
- HereBeBeasties 4y agoI 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.
- zadjii 4y ago> 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?
- aftbit 4y agoThat 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.
- carlmr 4y agoAlso 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.
- flamebreath447 4y agoI 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.
- shortstuffsushi 4y agoIf 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.
- rb2k_ 4y agoNah, 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.
- PAREL 4y agorb2k
- comfypotato 4y agoThe 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.
- joshuamorton 4y agoIf 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.
- TillE 4y agoYeah 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.
- comfypotato 4y ago10 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.
- bagels 4y agoMeta had the best code review quality I've seen in my career.
- nomel 4y agoAs someone who has only experienced terrible code reviews, could you give a little summary of what you liked?
- antiterra 4y agoThey probably had some pretty good cat meme image macros.
- bagels 4y agoYes good meme macros too. I left a few in my wake.
- bagels 4y agoCalling 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.
- jackblemming 4y ago
- dieselgate 4y agoBoo, please consider a review of the HN comment guidelines https://news.ycombinator.com/newsguidelines.html https://news.ycombinator.com/newsguidelines.html Could you potentially explain what makes it "some of the most garbage software imaginable"? edit: wording
- yodsanklai 4y agoMost 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.
- Jensson 4y agoSlow 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.
- H8crilA 4y agoIf 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.
- screamingninja 4y ago> If you do a break you might as well skim through emails and do some reviews. That's not exactly a break...
- H8crilA 4y agoWell, 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.
- pavon 4y agoWhen 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.
- BurningFrog 4y agoPeople 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.
- RussianCow 4y agoSome 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.
- BurningFrog 4y agoYou 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!
- ohgodplsno 4y agoHaving 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.
- zeckalpha 4y agoPairing can sidestep both of these problems. No context interruption and reviewed continuously.
- francis-io 4y agoI 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.