Calling equals on string literal

Question

I just was tidying my code a bit and there was this piece:

String saving = getValue();
if(saving != null && saving.equals("true")){
   // do something
}

Then I thought of doing it the other way around to get rid of the checking for null:

if("true".equals(saving)){
   // do something
}

It definitely works, but is this safe to do so? I mean string literals are stored in a common pool, while string object create by new are on the heap. But strings in the constant pool are also objects, right?

But still it doesn't seem like the right thing to do, even though it makes the code shorter.

Answer 1

Just to ensure there is a fully balanced set of answers to this question I would like to post a different opinion.

I think this mechanism is foolish.

If you have a null you should know as soon as it happens - hiding it will just postpone its discovery. If the null is not an exception, replace it with something else.

Taking this approach will fortify your code with what is called defensive programming where your mistakes are discovered as soon as possible rather than covered up until everything falls apart.

In summary - NullPointerException is your friend. You should use it to find mistakes in your code. It is very easy to use a Empty object such as Collections.emptySet() once you have determined that the null is not an exception.

Using the Yoda technique out of habit will inevitably hide errors you do not mean to hide. Not using it will expose errors much earlier. To me that is sufficient argument to not use it - ever.

To me - using

if(saving != null && saving.equals("true")){

means that I actually want to allow savings to be null and it is an acceptable situation - using

if("true".equals(saving)){

merely hides that deliberate choice in a way that could become a bad habit.