I've always had a problem with returning null from a method. And I was reminded of this when reading a post at Misko Hevery's blog about how to think about OO
I think returning null is a problematic, it's often abused as a way to have two different return types from one method. A quick googling of "returning null" has the suggestion of returning a null object implementation of your return type. Returning a Null Object is appropriate in some cases, but not when you need to signal to your client two different things. Misko's code refactoring show's an example of this. He refactors some login code (I really like the steps he took btw) down to something like:
Cookie login(Ldap ldap) {
if ( ldap.auth(user, password) )
return new Cookie(user);
return null;
}
When I read this code I see two things (structurally that is)
- If authentication succeeds then notify the client of the successful authentication with a new Cookie
- If authentication fails notify the client by returning null
What could a client of this method look like?
public void authenticateUser(User user) {
Cookie userCookie = user.login(ldap);
if (userCookie == null) {
//notify someone that auth failed
} eles {
//register them as logged in
}
}
We're doing the same work twice in each place, but with slightly different syntax, each place we need to check if login failed or not. What if instead we used IoC / "Tell Don't Ask" / "Hollywood Principle"
Cookie login(Ldap ldap, AuthenticationRegistry authenticationRegistry) {
if ( ldap.auth(user, password) )
authenticationRegistry.authSucceeded(new Cookie(user));
authenticationRegistry.authFailed(user);
}
and the client
public void authenticateUser(User user) {
user.login(ldap,this);
}
public void authSucceeded(Cookie cookie) {
//register them as logged in
}
public void authFailed(User user) {
//register them as auth failed
}
This new code is a little more complicated, but I think it is clearer, and more straight forward to implement. Now we have two entities that are communicating with each-other and we've defined the way in which they are communicating. Again I liked Misko's refactorings, I'd just take it one step further. It's all definitely arguable, but I think if you're ever in a situation where you want to be returning two types of data from a method using IoC is the way to go.
16 comments:
There is indeed a huge problem with how null is used in Java. I think you have correctly identified the problem, but not the solution.
The problem is that null is being used like an error code in C. It is being used for error handling. However, in java exceptions are a more appropriate mechanism for error handling.
You've also correctly identified that null is not a kind of Cookie! It always shocks me how few Java programmers understand this concept! Yes, making all reference types nullable breaks a static type system.
Historically null comes from C, where null is just a pointer set to the value 0. This is ok in C because C has (almost) no type safety anyway. C++ inherits this from C. However, C++ also adds references as a non-nullable replacement for pointers. Type safe languages like ML do not allow null, but instead have Option.
The reason that null is in Java is essentially because the guys who designed java are morons. They were just copying C++ without thinking about why C++ does things the way it does.
Anyway, your error handling method of using callbacks may work better in some circumstances, but generally just use exceptions when you want to indicate an error. That's what they are for.
Hi Brendan,
For me the question: "is a failed login an error / exception?". I don't think it is. This depends on how you define "exceptional or error" but for a login. I think it's expected that it will fail. There's nothing exceptional about it.
Using a checked exception in Java would definitely be better than returning null. And there are certainly cases where you want to do that, but in this case I think you're better off using callbacks.
Well, if you use callbacks it forces you to destructure the logic into a bunch of callbacks instead of just using straightforward procedural code like:
login()
do some stuff...
logout()
on exception:
cleanup...
In Java there's a lot of boilerplate in defining callbacks. You need a class and an interface. If I were using a language with first class functions like lisp, python, or even C I'd say the callback approach was more reasonable. I know that in Javascript people use callbacks for error handling a lot.
So I wouldn't say there's a hard and fast rule, but an exception looks better to me in this case.
Also, as for exceptions only being used for exceptional things... I would say it's a little more complicated than that. Exceptions should be used for error handling, and not normal flow control. However, if the function might be called in a loop where lots of exceptions adds up to a performance overhead, I'd provide a means of checking ahead of time to see if the operation will succeed.
Like:
while(...) {
...
if (object.canOp(args)) {
object.op(args);
}
...
}
where obj.canOp(args) returns true if object.op(args) will succeed. Optionally op can still throw exceptions in those cases where you don't need to use the canOp optimization.
However, in this case authentication is probably going to do IO, which is more more expensive than throwing an exception anyway, so the relative cost of doing so is negligible. I would say that authentication failure is definitely an error and takes us out of the normal flow of control that happens when operations succeed.
I gotta say though, I hate checked exceptions, maybe I've not seen them used well / successfully but my experience has been that they are bubbled up your call stack and pollute your code. I also think this is a question of style. Like you said it's more procedural to see everything together than smeared across some methods. It is certainly nice to see it all together but with a call back you just have to be concerned with implementing your methods and then you're done, it feels a little OOish (what ever that means).
Thinking about this more, I realized something interesting either you push down your call stack the client to be called or you bubble up the exceptions to be handled higher up.
As for exceptions being used for error handling not normal control flow, I guess I expect some things like a login failing to part of my normal control flow. It's not error handling or exceptional as far as I would think. It's a core part of my domain logic that I need to model.
I guess in this case that's what I like about the callback mechanism, it forces me to model that behavior.
And what really bugs me about checked exceptions is they effectively allow a method to return multiple types and force possibly complicated branching logic on the client.
I do think there are places for checked exceptions, and the finally block is certainly important. But in my experience they seem abused.
I think of null as a signifier for a non-object. Kinda absurd, but handy like the empty set.
In that view it makes no sense to ever return null from a method. Not just for the reasons already cited, but because it's nonsensical. What exactly are you telling the method's caller when you return a null? That something so nutty happened there's no object that can describe it? (!) The temptation to return null => you need to think a little more.
Exceptions are not a whole lot better in terms of avoiding abuse. It's another way to branch (and you know you've done the nested try blocks...just admit it :-). It seems to me that whenever you see a branch of any kind you should wonder why you're not using polymorphism, especially when you're the one designing these objects.
In the login example: who needs to know that the user successfully logged in? Why do they need to know that (what do they do with that knowledge)? Probably the caller of login() doesn't need to immediately know whether it succeeded, and several actors may need to know the login status. Probably most of the app doesn't care if a Cookie is the carrier of credentials; most actors probably just care whether the user is allowed to do stuff. All this suggests some amount of decoupling of the actual login() call, and its response, from the rest of the code. I like the idea of some Authenticator guy being asked to authenticate someone and to call back with whatever happened (like Zach's example is structured) because it allows for that decoupling.
Nice example of applying the "Tell, don't ask" principle!
What if I ask a StorageObject to retrieve an object and it doesn't find one? Eg getBlogPostById() and there's none to be found? Is it bad it returns null? If so, how could it be improved?
@Koen
You could handle this similarly to what I suggest in the post.
The method could be something like:
StorageObject.populate(StorageObjectAcceptor, SomeIdentifier) {
if (doesNotExist(someIdentifier) {
storageObjectAcceptor.requestedObjectDoesntExist(someIdentifier0;
} else { storageObjectAcceptor.accept(retrieve(someIdentifier);
}
}
Or something like that, with better names of course :P
Or you could throw a checked exception. Or something else wacky. I think anything is preferable to returning null.
Zachary: I'm surprised to hear you say you hate checked exceptions. In your blog post, you argue for a better way to handle the communication between callers and callees (great), but hating checked exceptions is basically similar to saying that you don't care to check if the result you are receiving is valid or not. You seem to hate checked exceptions because they force you to care about error situations, which is exactly the problem you are trying to solve in your post...
@cbeust What is an error condition? Is failure to login an error or part of you application flow? I'd say part of your application flow. And if it is part of your normal application flow then is throwing a checked exception appropriate?
What sorts of things do you think are good to use checked exceptions for?
Callbacks look very nice in the beginning. I was working once with >1M LOC in an application which had written the web layer with callbacks.
It was very very hard to trace, follow and understand the flow through the system for one request.
So I think the idea is a nice one, from my experiences with the callback system back then, I'd assume this does not scale to larger application flows and number of developers.
Hi, I think you've forgot a 'else' in the postrefactoring login method :)
Nice article though :)
I'd like to point to a different way looking what you described. A pattern originating in the Haskell language: The Maybe monad. In Haskel monads can be thought at chained up state transistions, every transistion performing some action. Authentication is some action, and maybe it succeeds. The idea of a Maybe is simple: A function maybe returns some value of some type (in this example an authentication cookie) as the outcome of some action or it doesn't. Depending on that the chain of actions takes a different route.
Let's for example assume a function taking a URL and Credentials, maybe producing an AuthCookie
authenticate :: Url -> Credentials -> Maybe AuthCookie
Later on the result can be pattern matched using the Just monad transform:
retriveData url authcookie =
case authcookie of
Nothing -> Nothing
Just AuthCookie -> ...
I think a lot can be gained in imperative programming, by looking at how the problem was tackled purely functional, and what can be learnt from this for imperative application.
@BluePyth no else needed because of the return on the line above it. Its naturally an else
I'd like to propose that the callbacks are unnecessary, and a problem in ownership (especially in a dynamically typed language) since the authenticate method is deciding how it can be used directly
better approach might be to have the authenticate method return a LoginStatus class which can hold state about success, time of login, and any other extraneous data. The caller can use that object and condition on it, leaving LoginStatus without the duplicated conditional
Just an idea - thanks for the post!
I would probably implement it with LoginStatus as 'Unknown';) proposed:
1. It returns status object instead of introducing output parameter -> AuthRegistry and calling it.
2. It adds new dependency too but it makes more sense to me that login action knows about status it returns rather than using some object caller gave it.
3. It's in fact similar to case with nulls but it adds lot more meaning to the code. You don't check null, you check loginStatus.isSuccessful().
Post a Comment