Last week I conducted yet another code review. While looking at the code I’ve noticed a lot of lines similar to If(x != null) and if(y < 0) all over the code. These lines were not “business related” but were put in the code to make sure that proper inputs were given to the methods. Usually this is a good practice but further review showed that some of the code was checking for conditions that could not possibly happen. it meant that a lot of effort was given to creating code that checks for things that would never happen.
I’ve asked the developer why he written that code and he answered that it was put there to make sure that he handles a certain scenario properly. I’ve asked him when such conditions would happen and after thought he answered – never!
In that case – I said – please remove this code. At first he insisted that the code should stay there because: You can never know and because it was already written. I didn’t even dignify the 2nd remark with an answer but instead told him to write a unit test that shows that the code was important and could save us from disaster. After a few tries he was convinced that there is no way to cause his methods to fail and that this code is not needed.
This is not a new thing, when I see a fellow developer over thinking some method or class I suggest that first he write a test. I try to make sure that all of teammates practice TDD (Test Driven Development) along other benefits writing the tests before hand guarantee that every lines written is needed. But even if you do not writes your tests first and you find yourself asking whether you need to write “that code” try writing a test before to show that there a problem there that need solving…
It seems that code coverage goes both ways – it’s important to make sure that your code is well tested but if there is a piece of code that is not tested it could be because it would never run – and as such can be removed.