I am all for writing code that is defensive. There are lots of little tricks as well as tools to check for mistakes. However, like most things you can take it too far.
I feel that I work best with examples, so here we go. Let's say we've got a method (in our code) with a signature as follows:
/** Returns a collection of Users. */
public Collection getUsers();
Here's code that I don't agree with:
Collection users = getUsers();
if (null != users) {
foreach(User user : users) {
if (null != user)
// do something
}
}
If we follow Item 27 from chapter 6 of Effective Java (wonderful book), then we're going to just return an empty collection so we don't need the first check for null. Since we control the code for the method getUsers(), we can make it part of the spec that it will not return null user objects, so the second check for null is also unneeded.
It's good when code guards against weird things, but in this case the code is potentially hiding wrong behaviour. Instead of putting guards in the client code, beef up the tests for the getUsers() to ensure that it will operate to whatever spec you create for it. The client code would cleaner and easier to read.
I see a lot of this. People would rather just hide the error, then have to deal with the real problem. The problems in this case are either a null returned from getUsers, where we should be returning an empty collection, and putting a null object into the collection. I think a lot of programmers like to just sweep the problem under the rug so that the user doesn't see any errors, rather than having to make sure things actually work right.
ReplyDeleteMy guess, is that the developer was having problems with getUsers, so instead of actually fixing getUsers, they figured it was just easier to put a couple if statements in. This can happen if getUsers is someone elses responsibility, and they figured it would take longer to get the fix than it would take to just write a couple if statements.
Checks to this degree are only necessary when you are dealing with user entered data, or data from a third party system which you have no control over. However you still see a lot of sweeping problems under the rug. Such as when the user was supposed to enter a number, and they entered something that isn't a number, so the programmer just decides that they should assume that means 0, or some other default value, instead of returning some kind of error.
In other cases, these "guards" in the client code can result in performance problems as well. Certainly an extra check for null won't be a perf problem. I agree with your main assertion that it affects readability pretty severely.
ReplyDeleteReturning null/nil for any modern OO language really isn't the best move for a method, especially when developers on the client end of APIs can do nice readability things like method chaining. A null in the chain throws the NullPointerException (NPE), which is a very bad runtime exception ... and then the software is in a bad and unrecoverable place.
It's interesting that you're going after it from the spec angle, as if the clients know what they want and they write a spec that the method follows. It seems far more likely that the method will be written from the provider angle, offering a method via an API.
The clients want the result of the method so it has to follow whatever terms are dictated by the API. If the API says the method could return null in some circumstances, then the client has to deal with that possibility. Same goes for exception handling.
If the client can make the assumption that the method will never return null, then the client code can be simpler. A good test suite will ferret bad situations out and expose possible NPE cases. Having access to the method's source code (white box) can also help, if you want to take the time to audit it.
I usually reserve these kinds of assumptions for methods I write myself. But not third party code, where I'm more defensive in the client code that uses it as well as the tests for the client code.
Ryan: Ya, I was using the word "spec" when I should have said "api". I see them as interchangeable since I think that an API is a detailed spec.
ReplyDeleteAs you mentioned, it's a pretty dumb thing to do in an OO language. I'd only check for null if the API explicitly mentions that it is a possibility.
If I was using a third party API that had that as a possibility, I'd consider using a wrapper around it so that I wouldn't have to splatter guard code though out my code - just in one place.
Kibbee: One thing that you pointed out: "This can happen if getUsers is someone elses responsibility..." is a good argument for not having code ownership. ;-)
I remember in our SEG 4th year project we were supposed to put wrappers around anything we used that was third party. By doing this, you now control the interface. Changes to third party libraries then only affect small parts of your code (the wrapper) instead of the entire project. If you're dealing with something that is likely to change, or that you may want to swap out later, it's probably best to put it in a wrapper.
ReplyDeleteDon't agree completely with this thing with nulls. I dunno, but I have my DAOs return nulls when no records are found. It's either that or an exception. Null works for me. I think that is what hibernate does too.
ReplyDeleteYa, hibernate returns a null or throws an exception (depending on the method you call: get or load) for a single element, but it does not return a null collection and it does not return a collection containing nulls.
ReplyDeleteMy rant / post was more about putting in guards when using an api for un-documented (and perhaps impossible) conditions and the dangers of that.
I agree with the approach that Kibbee mentions: only use a third party library in one place, instead of all over the code. Depending on how the app is architected this can either be done by isolating it in modules or by layering.
ReplyDelete