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…
Improving code review time
111–120 of 233 posts
Re: Improving code review time
#112there’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…
Re: Improving code review time
#113Earlier 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.
Re: Improving code review time
#114Am 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.
That's not exactly a break...
Re: Improving code review time
#115Here'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
Re: Improving code review time
#116Stacked diffs are awful compared to PR's. Not every commit should be clean
Re: Improving code review time
#117Re: Improving code review time
#118In 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)
Looks a lot more fun than code review to boot:
Re: Improving code review time
#119In 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)