4 ms·
The bigger problem I have with code review in general is that they are never about finding bugs. They almost always devolve into style concerns. Because style
by deckard1 10y ago
The bigger problem I have with code review in general is that they are never about finding bugs. They almost always devolve into style concerns. Because style is an easy, bike shed, thing. Finding bugs means knowing the context, and thinking about the code.
The best code review flow I've seen is where you put in a review, assign it to a specific person or two, and then merge that code.
What my current company has is the godawful github PR synchronous flow that everyone seems to love. This allows a few individuals to hold your commits hostage unless you bend over and implement their pet style. It pisses me off on a daily basis, and I'll probably quit in another month or two.
- cLeEOGPw 10y agoSo it's not a problem with code review. It's problem with the way people review the code. In my team we do discuss style here and there to keep it at least semi consistent, but we agreed to mostly check for possible bugs and/or maintainability. This means that a code review might actually be feature review where one person looks what had to be done, how was it done and maybe something is missing. So far it was pretty effective for us.
- StavrosK 10y agoWe leave style to the tools. We use pep8 and that's it. The commit won't even work unless we're pep8 compliant, and if the tool says you're ok, you're ok. I can't even remember the last time I saw a style-related comment on a PR.
- ytpete 10y agoHaving a team style guide helps a lot with that tendency: you get the argument over with once, team-wide, then write down the conclusion. So during each code review it's just a question of: does this match the style guide, yes or no? (Or you can enforce it automatically with tools like ESLint). And then the discussion can focus much more deeply on other aspects of the code.