DEV Community

Discussion on: 10 Principles of a Good Code Review

Collapse
 
mortoray profile image
edA‑qa mort‑ora‑y

I understand your concern about the product being useful. It's unfortunately common that programmer's produce things that don't actually work as intended, often because they didn't test it themselves, or there is a high-level compatibility. I have two approaches to get out of that environment:

  • Don't focus on low-level unit tests. Though they can be useful for debugging, they don't show much of whether something works. I'm totally happy testing low-level bits via their high-level function. I've been meaning to write an article about this a bit more... but the idea is that 100% isolated code coverage in tests is worthless compared to 10% high-level coverage.
  • You need a manual testing strategy. I cover this in detail in Improve quality and lower costs with assisted manual testing

I understand the problem you're solving with your approach. I agree you need a solution to the problem.

I guess trust depends on how well you know them. I primarily deal with a team I know. Our 3rd party contributions get a more rigourous review. But I don't mean about small details here, I mainly mean about the purpose of the fix. Certainly, even for code where I don't undrestand the goal I can still check several details of how it works. I can catch obvious failures even if I don't know.

For new vs. old code, yes, by all means assume the old code works. It's unfair to penalize pull requests because the old stuff needs improvement -- I even let some bad style slip through if it mimics the existing style. But there is some code that I just don't expect others to understand.

This is domain specific, and deals a lot with specialty algorithms usually. For example, I recently found a bug in the code I used to measure the length of vector paths. It took me a long time researching and finding the algorithms to begin with. Unless we want a reviewer to do the same research, and better, they simply would not have found the issue. They could understand the method names, and surroudning code, but the core algorithms present a bit of problem when it comes to reviewing.

For the same reason I just to have accept hacky workarounds #1 throuhg #7 on an Android target for our product. I assume the submitter did testing and research. I can verify the code is technically correct, ensure there's a manual test bit, but without spending lots of time I really can't say for sure if it's the correct approach, or even valid.

Idar Arye brings up a good point baout ROI as well. It's an unfortunate reality, that often it's more efficient, as a business, to ship buggy features (refer to Are we forever cursed with buggy software?. This is not an excuse though. The decision to trade priorites shouldn't be haphazard or done without thought.

Again, and this bears repeating: I agree code review should have rules and goals. I'm arguing only about some of the fine details here. Nobody should read this and come to the conclusion that the process is wrong. There are some details where I have alternate solutions, or have [hopefully] well reasoned objections.

Quality assurance is either a constant battle or it's being done wrong. :)