I asked what was ‘wrong’ in the following code. I had put ‘wrong’ in quotes, because it is an ambigious term. Here is the code:
if (null == sessionProxy.getShoppingCart()) {
throw new IllegalStateException("ShoppingCart may not be null");
}
One thing that we find here, is the use of a session proxy object. This is convenient, because it can do things for us at a centralized place.
So what is wrong?
In my opinion there are several things wrong with this code:
- violation of the Single Responsibility Principle
- dealing with null
How to fix this?
Fixing this may not be as obvious as how to detect flaws. This is existing code we are talking about, and you can’t just make changes without making sure you don’t introduce regression.
Solving the violation of the Single Responsibility Principle
We are reading the state of the sessionProxy to execute business logic. We are actually only concerned if the shoppingCart is set on the session. This could be via a null reference, but it could also be done in a different way. What we want is this:
if (!sessionProxy.isShoppingCartSet()) {
throw new IllegalStateException("ShoppingCart may not be null");
}
The method isShoppingCartSet() returns true or false. The implementation will look like this:
public boolean isShoppingCartSet() {
return getShoppingCart() != null;
}
The subtle difference is that we now have delegated the question “is the shopping cart set on session” to the class that is responsible for knowing, the sessionProxy.
Solving: Dealing with null
Another advantage by using the isShoppingCartSet method is that we minimize the amount we have to deal with null. We don’t need to check for null explicitly everywhere, we have centralized in one class.
Dealing with null can cause problems, the sooner you don’t have to worry about things being null, the better.
Instead of using a isShoppingCartSet method we could throw a checked exception in the getShoppingCart method. I like checked exceptions, because it forces you to deal with these exceptional situations. It also solves the null problem, as you know it always returns a value or throws an exception when it is (but should not be) null.
There is on caveat here: what if there is existing code relying on the value being null?
The real question is: What does the null value mean? Often it is abused as a status. Some people even use null as a “third boolean” (ie Boolean is ‘true’, ‘false’, or null for ‘unknown’).
As I see it you have an ideal path you want to execute, and then there are things that can go wrong and must be dealt with. In this case, we would expect a shoppingCart so we can do stuff with it. But, if it is not there, we catch the exception and execute other business logic we would otherwise have done with a ‘is null’ check. Ie:
ShoppingCart cart = sessionProxy.getShoppingCart();
if (cart == null) {
// do some business logic where cart is null
} else {
// other logic
cart.getSomeProperty();
}
turns into:
try {
ShoppingCart cart = sessionProxy.getShoppingCart();
cart.getSomeProperty();
} catch (NoShoppingCartSetException e) {
// do some business logic where cart is null
}
The second example clearly defines a ‘happy path’ (within the try). If you use checked exceptions consistently, you will notice your code will become easier to understand. Try it!
What if I don’t want to add an exception to the getShoppingCart method in my current proxy class?
In these cases I would suggest to create a child class of the sessionProxy, which does throw an exception. In the cases where you are absolutely sure that the shoppingCart may never be null, you can use this stricter version of the sessionProxy and deal with exceptions.
Another way of dealing with null – upon construction?
It is also possible to check for null in the constructor(s) of the sessionProxy class. In the case of a http session proxy, it would mean you have to make it immutable to make this work. The reason is that the http session is mutable, even once you have created a sessionProxy. Checking for null values at construction will not guarentee the values are not set on null later on. To fix this, you should create a ‘snapshot’ of the http session at time of construction of the sessionProxy. You read out properties, check for null and set values in private final fields. When you access the fields, you retrieve the fields themselves, and not access them via the http session.
Ie, this:
private final HttpSession httpSession;
public SessionProxy(HttpSession) {
this.httpSession = httpSession;
}
public String getProperty() {
String value = (String)httpSession.getAttribute("PROPERTY_KEY");
if (value == null) throw new PropertyNotOnSessionException("property was not set on session.");
return value;
}
turns into:
private final String property;
public SessionProxy(HttpSession) {
this.property = (String)httpSession.getAttribute("PROPERTY_KEY");
if (this.property == null) throw new PropertyNotOnSessionException("property was not set on session.");
}
public String getProperty() {
return property;
}
Concluding
A few lines of code, and yet so much to improve. We have found that:
- We can push the null check into a method of the responsible class. Making the class itself able to answer this business logic.
- We should throw an exception when we want to return null. Dealing with null is hard. When you don’t have to deal with null, your code will get much easier.
- When throwing an exception has too much impact, create a child class which can throw exceptions to reduce impact on current code.
- Checking for null upon construction, for a session proxy, is not possible. Only if you create a DTO out of it (but not a proxy).
