[hangonhn]

I would go further and say that you should never trust code reviews to catch bugs. They're great for all the reasons in the article and what "zhemao" said but they're horrible for catching any bugs beyond the most trivial ones. Humans simply aren't good at running code in their head like that. Code review is no substitute for good code coverage and automated testing, preferably pre-commit.

[jakub_g]

Honest question: what strategy would you recommend to deal with a (senior) developer who specializes in opening gigantic pull request with significant number of bugs? (I invest a lot of time to read through the code and I catch lot of stuff, but it's draining huge amounts of my energy). Declining anything with coverage below 100% is not a viable option unfortunately, I think, and preaching about best practices gives little effect.

[yxhuvud]

"This commit does too much different things. Please break it up into smaller units that do one thing only."

And then continue to reject it until you get nicely chunked up stuff.

[timmy-turner]

I'd probably make your work visible by counting the hours spent in the review each day or week. If you are able to roughly calculate the costs associated with those reviews, you may be able to push your organization towards adopting better coding or testing standards. Alternatively, instead of reviewing the code and pointing out the bugs, suggest or write a few tests which will uncover those. Showing good test coverage in action may be better than just preaching it.

[hangonhn]

One of the really neat side effects of unit testing with high coverage is that it means your components must be suitable for more than one client: your actual product and the test cases.

This means it decouples all the components which leads to check-ins or pull requests that are also self contained and modular.

If you push for good test coverage (less than 100% but more than 80% perhaps?) for your unit tests, you'll get less bugs and more modular code and smaller check-ins.

[lisivka]

If you see a problem, then describe the problem and reject request.

[cordialpanda]

There could be a bunch of reasons there's bugs.

I view bugs as a potential issue in an individual's development process. It sounds like the biggest reason you already hit on -- gigantic pull requests. Gigantic requests usually mean PRs that aren't focused to one discrete work-piece, but represent work that kinda meanders to completion. Make sure they're focused, and ask for nonessential pieces to be delegated to another PR (style-only PRs being a good example).

Ultimately the only way to suppress large pull requests (or problematic practices in general) is to evangelize simple development workflows that others can help you with. Keep in mind that change for anyone, no matter intelligence or stubbornness, takes time - culture especially. You need to make it easier for them to adopt your methods instead of theirs when the time comes and they get fed up and are ready to change. Make sure you don't have an antagonistic relationship, either, or they'll do anything but what you want just to spite you :(

I've broken up some stuff to look for in code reviews, in code bases, and in developers. You probably only want to look at developers.

For the code itself:

- Do we have overdeveloped patterns that get in the way of expressive code? - Do these bugs fall into a general classification that can pinpoint a source (such as timing issues, authentication, database calls, or info validation?) - Is your code-base DRY? Repetitious functionality sprinkled around will mean bugs that never get fixed. Eliminate repetition. - Age of code-base? Newer code-bases should be more accepting of large PR's/more bugs, but should compensate with refactoring-only PRs, test blitzes, and accessible code coverage. Old code may have traditions that have outlived their usefulness, and owners that have grown too accustomed to the state of the code to understand the need for improvement. - No experts. Is this an inherited code-base with poorly documented patterns whose original authors have moved on?

For code reviews (speeding up review):

- First read-through of the code is only to familiarize myself with the code added. - Does this PR solve one problem or a bunch of problems? One feature/bug/style-change == one PR (per repository). Ask to break it up if it's big and doing more than one thing (unless those things are highly coupled). - Is there core functionality? Start with the most reused piece, and critique that first. - Is this code shared? Shared code should be held to a higher standard, especially if other people depend on it. Get their eyes on it too, it takes some work off you, builds ownership, and if this person is a problem, you'll want allies to back you up. - Are methods long? Longer methods tend to do more than one thing, and invites glomming onto existing methods instead of creating your own. Higher standards for shared code modification tends to lead to shorter methods, because reusing code needs to be a deliberate decision. Plus long methods can hide code reuse. - Is manual testing difficult? Long or difficult verification loops tend to allow for more bugs.

For the developer:

- What are their tools? What tools are you using that makes you more effective? Learning new tools is hard, but most bugs come from ineffective development practices - Are they senior in knowledge or senior in age/time? Not all senior devs deserve their title. - Do they stay on-task? Do they have to thrash between tasks? Unfocused development is a big source of bugs. - How familiar are they with the code base? Senior or not, code from new devs should require more scrutiny (but not to the point that it discourages collaboration).

[mikestew]

That was going to be my comment on the title. If your code reviews are "...just for catching bugs", you're doing it wrong. I put another comment in this thread describing a crappy line of code. It works (I wrote the test for it), but my...$DEITY could it have been easier to read, and made more maintainable. But it apparently sailed right through code review.

Like you, I'd say that code review isn't for catching bugs at all. Is it readable? Can some Shmoe off the street who is hired to maintain make sense of it? How's the complexity? Do I need to maintain eight different states in my head to grok it? Et. al. Now, a team can help prevent potential bugs (either outright as the code stands, or bugs introduced later when someone tries to maintain it and it's a pasta derivative) with code reviews. I mean, didn't we run the unit tests we wrote before we submitted it for code review? Why are we finding bugs just by looking at the code?

[pmarreck]

I've personally (YMMV) found that going all the way to full TDD has resulted in the fewest delivery bugs (by a lot). Beats out unit testing for sure, but either of those are miles ahead of NO testing.