What defensive programming is and isn't
This is a tech rant so ignore it if you aren't interested.
Defensive Programming
First, a definition (my understanding):
Defensive Programming means:
- Not trusting the inputs to your methods/modules and validating them - throwing exceptions for unexpected input.
- Every if should have an else. This is a caricature on my part, a more pragmatic/sensible approach is if you have a lot of if ... else if ... or a case statement, always put a trailing else or default that throws an exception if you think you have handled every valid case.
This is programming by contract/invariant under another guise, perhaps a little simpler. The second point can be a total pain if you use testing tools that insist on testing every path through the code, because you have to somehow pass invalid input through.
Look at the following allegedly defensive code. I would argue that this is not defensive, because it does nothing with the invalid input:
void bing(LogObject logger) {if (logger == null) {
System.err.println("logger was null");
}
LocalLog log = logger.getLocal("bing");
// ... blah
}
So ... you've logged that the logger is null but carried on to where you'd get a null pointer exception. Why? Either throw an exception, or let the exception throw itself when the getLocal method is called. I have spent most of the last 2 days removing this kind of nonsense from thousands of lines of code. Let's look at another example:
boolean isSet(String testString){
if (testString == null)
{
return false;
}
return (testString != null && testString.equals(Constants.ON)) ? true : false;
}
Assume that Constants is a class that has some string constants defined in it.
I want to kill you! NOW! This is one of the worst examples I've seen of just not thinking properly. The defensive part is trying to handle the potential null, but it has been done in a really idiotic way:
- Why test for nullity twice? Will something change the value of the object reference before it gets to the return statement? Is the Java virtual machine inhabited by pixies that change what an object reference points to between statements just for fun?
- The expression inside the brackets by the return statement is already a boolean - why use the conditional expression?
if ( true ) return true ; else return false ;
Are you insane? I've seen this a lot in Oracle PL/SQL code as well. A boolean is a boolean is a boolean.
- If you have read Effective Java you will know that the equals method tests for nullity in the passed object anyway. The problem is the potentially null testString will not have an equals method. You do trust that your own constants are not null, pixies excepting. So rephrase to
return Constants.ON.equals(testString)
This is defensive because it handles the null case and it also handles all of the cases where what has been passed isn't a string (not likely in this scenario though). It's also clean and not obscured by silliness. This leads to a common Java idiom where you always put the constant first and call its equals method, rather than the more natural passed object.
Stop returning null
A really stupid Java idiom is returning null if you've hit some kind of error condition, rather than throwing an exception. I've seen reams of this nonsense:
try {
// some really dangerous operation
}
catch ( Exception e )
{
// Which one of the 5 exceptions was it? you may want to handle them differently
e.printStackTrace(System.err) ; // least it was logged somewhere !!
return null ; // or maybe a runtime exception??
}
Just propagate the exception or roll your own generic one for your app and throw it after encapsulating the real one. This also means the code that relies on you has to keep checking for nullity or throw random null pointer exceptions. This is inexcusable laziness and results in code like I showed before that tests for nullity all the time. Without an exception we don't know what the nullity means, so we can't fix it without dredging through the code (which may not even be ours) and put print statements everywhere. Usually these statements are not deleted and everything starts to bloat, especially your log files. Using nulls like this breaks the contract with the caller of the method. Exceptions are part of the contract. The user can choose to ignore them, but you kept your word with them.
So, stop returning null, please. If it's an error then let it be one. Returning null breaks the encapsulation, because you don't know what happened without having to go into the offending code.
If you are working with Strings, and you think it's OK to return an empty string (as in a find method that didn't, say) RETURN AN EMPTY STRING!!! How radical is that? None of this crap
String myVar = params.get("myVar") ;
myVar = (myVar != null)?myVar:""
Which I have written. J2EE designers, yes, I hate you for wasting my time. I end up writing a helper function to encapsulate this nonsense every time.