[deleted]
Improving code review time
211–220 of 233 posts
Re: Improving code review time
#212I'm a strong believer in fast reviews. I get into deep working mode for 3 hours a day total, on a good day. The rest is meetings, daily sync, coffee, lunch, "hey can you look at something", hallway conversations, emails, my own inability to concentrate when I'm not feeling it. I've been in this industry for coming up on ten years. None of this is going to change, unless I become an academic or a hermit. I don't get t…
> I WILL merge your change if you pass my review.
Doing that, you remove the possibility at the submitter of a final check before the merge. Especially if the merge happens on another day, rechecking with a clean mind helps to find missed things.
Then someone does that to my changes, they get a kind message to not do this anymore.
Re: Improving code review time
#213Earlier quoted context omitted.
One thing I've enjoyed where I am now is that PR comments come in two flavors. The first, actual feedback. The second, borderline pedantic issues that are prefaced with "nit: " in the comment. Nit comments are safely ignored but are there so that if the author wants to put in that change while changing some other issue, then OK.
I especially like Github's new option to make a _suggested diff_ of what you want changed. Typo fixes, comments re-worded, etc. It really reduces friction, both as the person making the suggestion, and as the person who authored the PR.
I completely agree, suggesting the actual diff is the ultimate "put up or shut up". One of the most annoying review situations is when you get some vague comment that doesn't spell out what change is being asked for.
Although the best response to that sort of thing is just a quick DM - "hey, I'm not sure how to interpret this comment, wanna hop on a call for a minute and explain?"
Again - put up or shut up. Want a change? Cool, let's pair on it. I want the code to be good too. But I'm not going to just read your comment and based on the vibe I'm feeling shoot off some change, only to find out that it wasn't what you meant.
Re: Improving code review time
#214I'm a strong believer in fast reviews. I get into deep working mode for 3 hours a day total, on a good day. The rest is meetings, daily sync, coffee, lunch, "hey can you look at something", hallway conversations, emails, my own inability to concentrate when I'm not feeling it. I've been in this industry for coming up on ten years. None of this is going to change, unless I become an academic or a hermit. I don't get t…
> I WILL merge your change if you pass my review. Doing that, you remove the possibility at the submitter of a final check before the merge. Especially if the merge happens on another day, rechecking with a clean mind helps to find missed things. Then someone does that to my changes, they get a kind message to not do this anymore.
Of course, this is only if the submitter explicitly agrees to that.
Re: Improving code review time
#215there’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…
They measure the "quality" of review based on time spent looking at the code which suggests to me they have absolutely no idea why they are making people do code review at all. This is an especially bad metric because 1) We have good data that after about 60 minutes of reviewing we start to lose the ability to find more issues. 2) It incentives making bigger and harder to review changes so that people spend more time…
You seem to suggest that the team is trying to increase eyeball time metric, but that isn't what the post said. The post said that they wanted to decreases the time to review metric, but without changing the eye ball time metric.
Re: Improving code review time
#216Earlier quoted context omitted.
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 m…
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.
Re: Improving code review time
#217Earlier quoted context omitted.
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.
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.
Re: Improving code review time
#218Earlier quoted context omitted.
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!
Re: Improving code review time
#219Earlier quoted context omitted.
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.
Reading is absolutely subjective, but that doesn't mean it can't or shouldn't be optimized for (just like with regular written language -- editors are a thing).
Being a good code reviewer (just like being a good editor) is absolutely its own skill though, and people can be bad at it.
Re: Improving code review time
#220Am 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 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…
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.