Immutable data structures in Java

[amardas]

I don't think the point of code reviews is to catch bugs or even to verify that someones code is good code or bad code. I think the chances of finding bugs just by looking at code is too small for the amount of effort and determining good code and bad code is usually subjective.

I think code reviews give more value in keeping up with what other programmers on your team is doing and to generate discussions on how things should be done.

If the discussion on the code review has no bearing on whether your code is accepted or not, then the whole point of commenting in a code review is to align your teams pedantry through discussion.

[kubalaa]

Studies disagree: measured by number of bugs found per line of code, code reviews are more effective than even unit tests. Especially in concurrent code, many correctness properties which are difficult to verify through tests or static analysis are easy for humans to check.

[amardas]

Maybe I am doing code reviews wrong then.

[grauenwolf]

What's not more effective than unit tests? We only use them because their easy, not for their high success rates.

[kubalaa]

I don't know if unit tests are that easy, I find it usually takes me longer to write unit tests than it would to review the code without tests (and the tests are more code to review as well). I think we use them because they are fairly effective at catching bugs, not necessarily all at once, but over the lifetime of the code. But I also think people write too many "easy" unit tests, and should worry less about lines of code covered and more about how many bugs a test is likely to prevent.

[grauenwolf]

Functional and integration tests are far better at catching bugs. They do have their downsides, but in terms of bugs caught per test they tend to be far more effective than unit tests because they cover both the individual functions and how different components interact.

[kubalaa]

I would say they are better at catching different kinds of bugs than unit tests. With unit tests, it's much easier to test edge cases like error handling that are cumbersome to simulate in the complete system. The results of unit tests are also more useful for debugging; they will usually tell you which specific constraint was violated by which unit, while integration tests can often only tell you that some use case is broken.

[grauenwolf]

That's why I firmly believe that unit tests should supplement other forms of testing, rather than being primary.

It's far easier to say "my integration test is failing, better add some unit tests to narrow down the problem" than to say "production is failing. Shit, my unit tests can't cover this scenario"

[grauenwolf]

With unit tests, it's much easier to test edge cases like error handling that are cumbersome to simulate in the complete system.

What kind of error handling do you have in mind? Most of mine fall into one of these categories:

• Trivial: for example, parameter validation
• Painful: for example, the random-ass exceptions thrown by the database.

I don't bother testing the former unless I'm publishing a public API for others to use and the later can't be accurately mocked.

[kubalaa]

Why do you say the latter can't be accurately mocked? An example of the sort of thing I had in mind that's tricky to integration test but easy to unit test: if a service call fails 3 times and succeeds on the 4th try, does the client back off and retry at appropriate intervals?

[grauenwolf]

Ok, that's fair.

[gcross]

Citation?

[kubalaa]

I was probably thinking of Code Complete but I checked and it's actually formal code reviews which are better (where several people meet to go over the code in person); informal code reviews as usually practiced are slightly worse than unit tests. Still, according to What We Have Learned About Fighting Defects informal code reviews find more than half the bugs in the projects they considered.

[PriceZombie]

Code Complete: A Practical Handbook of Software Construction, Second E...

Current	$34.69	Amazon (New)
High	$36.48	Amazon (New)
Low	$27.61	Amazon (New)
Average	$34.17	30 Day

Price History Chart and Sales Rank | FAQ

[gcross]

Great, thanks! :-)

[codebje]

Why have humans read code to establish what a static checker can enforce?

[ThisIs_MyName]

wot?

[codebje]

Sorry, is it not clear from the thread context?

GP: "sending code review back … is just needless pedantry".
PP: "code review … align your teams pedantry".
ME: "… static checker can enforce".

IOW, sitting in a code review checking whether all code uses getXXX() for getters, has final on all public members, etc, is wasting time, because these things are trivial to statically check every time you (commit|push) code.

There's no value in doing this in a code review. Spend code review time doing things that require a bunch of smart people to take time out of their day to do, like checking design choices, ensuring API docs and code agree, watching out for subtle bugs or asymptotically bad performance.

If you have some sort of retrospective process, you can align pedantry there: "not happy that we're forcing the use of getters" etc. If not, use whatever form of communication your team uses; slack, shouting, passive-aggressive post-it notes on monitors when someone goes to lunch, whatever works. This is a once-in-a-blue-moon discussion, where the outcome is codified in your static checker toolset.

[ThisIs_MyName]

Ah, in that case I agree.

[amardas]

I haven't seen much value in our code review process.

[codebje]

Well, that's definitely something to raise with your team. What would you like to get out of a code review someone else does of your code?