Sunday, 10 August 2008

Returning 'null' Considered Dishonest

Background


Antony Marcano and I have just started running a coding design workshop. Most of the audience are new to coding and we are trying to focus on good habits that are applicable across all programming languages.
In our first session, we created a vending machine. By the end of 90 minutes, it was able to dispense a drink as long as it had sufficient money and stock available.
One of the questions that we asked was "What do we do when the customer has not inserted enough money and we press the button for the drink?"
Some of the people who had some programming background said "Return null", which is what brings us to this post.

Good Citizen


In the wiki for the PicoContainer project, there is a page titled "Good Citizen", which details a number of good practices that all Java classes would benefit from trying to adhere to.
The practices make a lot of sense when you think about them, but they aren't really explained. I'm going to try and address that issue as we cover them in the workshop.
The practice that we are looking at today is "Never expect or return null", with a dash of "Fail Fast" for flavour.

What's so bad about null?


In the Java world, when we declare a method signature, we specify a return type. In the case of our vending machine we had:

public Drink giveMeADrink() {...}

By declaring that the return type is Drink, we are signing up to return something that "is-a" drink.
We could also return null, which is a nothing value. We could use this to represent that you did not get a drink.
The client code may look something like this:

myVendingMachine.giveMeADrink().downInOne();

If we return null, this code will fail with a NullPointerException. Not particularly useful, but at least we are using the result straight away. The problems become much worse if we store the returned Drink for use later.
When we said we will always return a Drink, we lied.

Programming Defensively


The sample client code above makes the assumption that the result of giveMeADrink will be a Drink. Given that we've actually signed up to that contract, that doesn't seem to be unreasonable. But now the client code is broken and they have an angry customer, they are going to have to work around the issue. It would probably look like this:

Drink myDrink = myVendingMachine.giveMeADrink();
if(myDrink != null) {
myDrink.downInOne();
}

This code is actually saying "I've asked you to give me a drink, but I don't trust you, so I will check first".

Why isn't this working? An Exceptional Approach


If we rely on our client to check that they received a valid result, we lose out on an opportunity to let the client know why the call was unsuccessful.
In the Programming Defensively example, we can recover from being returned a null, but we don't know why it was null. Was it because we hadn't inserted the money? Was it because the machine was out of stock? Was it because the stars were out of alignment?
Do we handle the scenarios differently? If we haven't inserted enough money, that's something we can deal with, but if the machine is empty, we need to look for another machine.

What if our code looked like this?

public Drink giveMeADrink() {
if(weDontHaveEnoughMoney()) {throw new NotEnoughMoneyException();}
if(weDontHaveEnoughStock()) {throw new DrinkOutOfStockException();}
return new Drink();
}

What we have said is "We will always give you a drink or tell you why we couldn't"

Now when we attempt to call giveMeADrink, it lets us know straight away if it can't proceed. It also gives us a good indication of why it is having problems.
The client code calls:

myVendingMachine.giveMeADrink().downInOne();

and gets told "Sorry, I'd give you a drink, but you didn't insert enough money".
Our code is being honest, polite and giving the client an opportunity to remedy the situation. The customer is still angry, but now he's angry with himself for not putting in enough money.


In Summary



  • Programming defensively is programming distrustfully

  • Returning null is dishonest. It requires others to check that we've upheld our side of the bargain

  • Throwing a meaningful exception allows us to let the caller know why their call did not succeed

15 comments:

Kris said...

One way or the other, I'm going to get an exception. Your way will be slightly more informative, but what is the use of that extra information?

I agree with avoiding nulls in a codebase if possible, but I hesitate at saying that we should define a new exception type for each possible reason a contract is broken. It's more effort and maintenance to handle all those exception when the error condition probably should occur unless by a programmer mistake in the first place.

It's an old debate to be sure, but what's the strengths and weaknesses of:

if (weHaveEnoughMoney()) {
    giveMeADrink();
} else {
    blinkNotEnoughMoneyLight();
}

versus:

try {
    giveMeADrink()
} catch(NotEnoughMoneyException e) {
    blinkNotEnoughMoneyLight();
}

eckes said...

I had also a discussion about this anti pattern on my german blog. But my main concern is not using return as a documented response value, but my problem is that often code errornous returns null because of lack of branch coverage.

So my main rule is: if you want to return null, you need to documen that and you need to use "return null;" to make a reviewer obvious in which cases you expect this return value.

http://itblog.eckenfels.net/archives/352-Null-returns-in-Java.html

Besides that I try to avoid null returns (for example with empty collections).

Jeremy D. Frens said...

@kris: There are two reasons why I would argue in favor of at least a few new exception types.

First, I've found that once you start returning null, you end up returning null a lot of places. You end up with lots of null tests.

Second, throwing the exception as soon as the problem is detected yields better debugging information. A null pointer exception may not be triggered until some time later in a much different part of the code.

Instead of throwing exceptions, though, you could use a simple hierarchy with a Null Object. So instead of returning a Drink, return a MachineResponse, an interface which is implemented by classes Drink and NotEnoughMoney. I've found an approach like this usually pays for itself in the long run in terms of expressiveness and extensibility without costing too much.

Mark Needham said...

Interesting article on a subject for which there seems to be no clear guidelines. As Kris mentions up the top it seems that we are just handling the case where a drink is not returned in a very similar albeit more intuitive way.

Would the exception type always be checked or would a runtime exception ever be a better solution?

When would it become the case that it's better to return a Null Object pattern variation of the object rather than throwing an exception?
In this case maybe throwing the exception is good but clearly you don't want to become too dependent on handling the system's state through the use of exceptions.

Sarah Taraporewalla said...

Hey Andy,
I was with you all the way until your preferred solution was to throw an exception.

IMHO Programming by exception is never a good idea. Exceptions should be used only for really weird debugging situations and not to return business logic.
Is there a chance that giveMeADrink() will not return a drink? Then instead of the method throwing a tantrum and yelling at the caller that there was not enough money, it could return a response object, which details the reasons why It can't.

As @jeremy points out, a better way would be to return something like a Maybe object, which has two implementations - when it can return a drink, it returns a Something which contains the drink. If there was a problem, it returns a Nothing with perhaps a message detailing why.

Maybe[Drink] giveMeADrink(){
if (weDontHaveEnoughMoney) return {new Nothing(notEnoughMoneyMessage)}

if (weDontHaveEnoughStock) return {new Nothing(weDontHaveEnoughStockMessage)}

return new Something(new Drink())
}


Maybe[Drink] drink = giveMeADrink()
if (drink.IsPresent) { ... do what you will with the drink.Value (in the something case, it will be the drink object) .. }
else { .. do whatever you want with drink.Value (in the nothing case it will be the message) ... }

Andy said...

@Kris: Where does your sample code live? It seems that it's in the vending machine, in which case, how does blinkNotEnoughMoneyLight() keep to the contract of returning a Drink?
If it's in the client code, then "Tell, Don't Ask" says that we should not base what we ask of the called object on the state of that object. That's a decision for that object to make. Let's just ask it and see what we get.


@eckes: I'm thinking that we might move to a DrinksChute object, which is a collection, but that's not the way it is at the moment

@jeremy: Null Object is a possibility, but it depends on the planned usage of the returned object. In this case, the Drink has no behaviour, so it doesn't make sense to have a NullDrink. If we can't return a real Drink, I want to know why

@mark: These exceptions are unchecked :-) I'll post some thoughts on that soon.
Null Object is useful where we are expecting some behaviour from the returned object rather than just existence.
A quick example that I thought of would be a NullTransaction on a BankAccount. This would have no effect on the balance of the account, but may be used to make a note in the transaction history.

@sarah: I see where you're coming from, and there are many possible workarounds, but I have asked the VendingMachine for a Drink. I don't want my clients to have to worry about DrinkResponses that may or may not have a drink, which then has to be extracted. The DrinkChute collection would be acceptable, but it's not the simplest thing in this example.

@all: This is a contrived example, but the exceptions aren't really being used for business logic. They are unchecked, so there is no reason to catch them unless you want to handle the situation. The system fails fast with a clear reason as to why and where the problem occured.

Obviously, this is all just my opinion :-)
I'll defend my choice of using Exceptions with this:
The client code needs a Drink to proceed, it makes no sense to continue running if we do not receive a drink. If we use any of the other solutions, we still have to check to see if it's ok to proceed (did we get our drink?). With the Exception route, the application stops and tells you why, without additional burden on the client.

ben.biddington said...

And provided are informed how to avoid NotEnoughMoneyException, then they need not use try/catch as control flow.

Kris said...

@Andy

The blinkNotEnoughMoneyLight() would live in the vending machine. I use it to bring up the point of how the code is intended to react to being able to give out a drink - blinking a light to the user indicating more money is needed.

There is no contract that there should always be a drink from a vending machine. There is probably a business rule that says that once the needed money has been given. The value of null would only given out when that precondition is violated, which would only happen from programmer error.

Brian said...

This post is making me thirsty!

Jamal Mavadat said...

@Andy
IMO classic programmers are usually implementing C-Style control flows! Programming languages with exception support however add more convenience and support to the case, BUT do not replace null-reference handling. Here we go (I know this isn't the perfect example):
if(( machine = getActiveMachine()) == null) { ... }
else if(( drink = getMeADrink() == null) { ... } ...
In the above example we don't expect something "exceptional" and everything is checked sequentially branching should a particular condition triggers - this is supposed to be the normal flow.

BUT exceptions are beautiful if it's really considered an "exception" and should pick an alternate flow instead of the regular normal flow. For instance, designer of openFile( ) method MIGHT decide to throw an exception if invalid filename supplied!

Good design employs a mixture of both types; the point is to properly decide what should be considered the "normal" flow and what should be the "exception" or "error" flow! The rest is THE ART OF DESIGN... in summary I stand for proper use of null-handling and exception-flows, not eliminating any of them...

Andy said...

@Jamal, thanks for commenting.
I want my code to fail fast. If I don't get a Drink, I want my code to tell me why.
I don't want my client code, that is expecting to receive a Drink, to have to check that I didn't cheat him and give him a null instead :-)
Can you give me an example of when a null is a valid response to something that is expecting a Drink object? That is, can you describe a situation where an object that needs a Drink to work with, would be happy with a null?

Jamal Mavadat said...

@Andy, you're most welcome matey, my pleasure for being here :)

Andy said: "I want my code to fail fast. If I don't get a Drink, I want my code to tell me why." As said before, both approaches can co-exist, and now you described a situation in which you shouldn't be using null-refs! Perhaps solutions such as response-types (normal flow with additional info), or exceptions (exception or error flow optionally with yet more info).

Examples for returning nulls (sure we have alternatives):
1- getDefaultDrinkOrNull( ) if no-default-drink is considered a typical response and we prefer handling it in normal flow, fortunately designer has cleared all potential confusion by proper naming.
2- in a linked-list I may like getNext( ) return next object or null for the trail gifting more simplicity for iterations.

I think the "drink" example is a bit too generic! We could better be discussing null issues by real-world scenarios. For example:
A- If the Drink type is actually a Customer in a web scenario then returning response-types or exceptions are probably better approaches.
B- In a getTempFile( ) : File I might go for exceptions if it cannot return a temp file!
C- And for getActiveUser in a security service I might prefer returning null for indicating no-active-user message - however, I myself have designed security services in which active users were returned by response-types and some specific exceptions too. My mind wasn't changed, they were just proper solutions for their unique requirements.

All I try to say is, we should NOT replace null-handling, BUT we may discuss a particular case in which null-handling is not the best.

Fredrik said...

Calling Drink() when the vending machine has not yet received enough money would be an invalid operation for the current state of the vending machine - so f.ex throwing an InvalidOperationException would be OK here, imo.

If you don't want an exception, you could use TryGet pattern:

Drink drink;
if(vendingMachine.TryGetDrink(out drink))
{
// we got a drink!
}

Jamal Mavadat said...

Try-get pattern is supposed to add a hybrid support!!! A solution to another problem...

valentin said...

This is an interessting discussion.

I tend to agree with the "use a maybe-object"-approach for recoverable failures. I think the usage of exceptions should be truly exceptionel denote cases, that the programm can not recover from on its own (Filesystem full, Connection to DB Server lost, Drink machine empty, No electricity .. oh wait ..) of course these kind of exceptions should be unchecked, so that you don't need try-catch on every layer of your design.

In this case however it is a failure on buisiness abstraction layer that should be dealt with in a more regular manner. A "maybe object" might be a little code overhead, but in the end it is more natural for the case where there is not enough money inserted. Key to this decission is: the user can know how to recover from this illegal state.

I think the code should however throw the unchecked exception if the machine is empty. The user can not do anything about it beeing empty and it is impossible to recover from this situation (until the company comes and refills the machine that is).

Have a great week,
Valentin