Wednesday, September 23, 2009

Auto-unboxing hazard -- the three states of Boolean: TRUE, FALSE, and...

What do you say about the following code:

boolean isActive = (Boolean) pageContext.getAttribute(IS_ACTIVE);
if(isActive) {
// dosomething
}

If you are worried about the casting to Boolean in case where attribute IS_ACTIVE doesn't exist, don't worry, it's OK to cast null to any type, null stays null.

So, is it OK?

Well, no. The problem is with the auto-unboxing to primitive type boolean, which throws NullPointerException if the Boolean is null. Which is a bit hidden from the programmer who might miss this hazard when writing the code.

How should we fix it?

Is the following a decent fix:

Boolean isActive = (Boolean) pageContext.getAttribute(IS_ACTIVE);
if(isActive) {
// dosomething
}

If you ask "what's the fix?" - we turned the variable isActive to be Boolean rather than boolean. Which should eliminate any NullPointerException on the first line! But only in order to pass the exact same NullPointerException into the 'if'...

So, what about:

Boolean isActive = (Boolean) pageContext.getAttribute(IS_ACTIVE);
if(isActive.booleanValue()) {
// dosomething
}

Yes, you are right, same NullPointerException. Maybe now even more explicit.

We could go with:

Boolean isActive = (Boolean) pageContext.getAttribute(IS_ACTIVE);
if(isActive != null && isActive) {
// dosomething
}

But I believe this looks better:

Boolean isActive = (Boolean) pageContext.getAttribute(IS_ACTIVE);
if(Boolean.TRUE.equals(isActive)) {
// dosomething
}

No comments: