CODEWITHSUNDEEP
javastringcomparison
Kuba Spatny
Kuba Spatny

Reputation: 26990

Calling equals on string literal

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.

Upvotes: 26

Views: 21881

Answers (8)

Maroun
Maroun

Reputation: 95978

I disagree with some of the answers. Sometimes you do want to know if your variable is null. It might be an indication that something bad is happening in your code and you might want to reconsider your logic.

I don't like to do if ("someString".equals(myString)); because I do want to know if myString is null and maybe handle it or change my logic to prevent similar situations.

Imagine you have an array of objects that was miscalculated due to some bug:

myStrings = {..., ..., null, null, ...};

Then you want to perform some operations on the objects of the array, and you'll have:

if ("myString".equals(myObjArray[i])) { ... }

You won't get NullPointerException and you might think that you performed the operation on all the objects in the array, and you'll never know that you miscalculated it because you hide the exception. Whereas if you had:

if (myObjArray[i].equals("myString")) { ... }

You'll get an NPE and you'll know something is wrong in the array, and you might want to fix this or handle it in another way.

Upvotes: 11

schlebe
schlebe

Reputation: 3715

if ("true".equals(saving))

is perfectly "save" and recommended by dummy product as SonarLint and any other advisors that establish this "well" thinked rule that your chief request that you follow.

I say stupid and my explanation is the following.

What happens if you have following code

if (!saving.equals("true"))
    {
    if (saving.length() > 10)
        {
        // do something
        }
    }

You can use following advised code 

if (!"true".equals(saving))
    {
    if (saving.length() > 10)
        {
        // do something
        }
    }

But when your code is deployed on production, the program will crash on length() function if saving variable has not been initialized and contains NULL.

You have changing your code to avoid NullPointerException on equals() function but your program will return another NullPointerException on length() function !

The good reaction is to stop to use String literal tip and to analyze code to find a perfect solution as in the following example.

if (saving != null)
    {
    if (!saving.equals("true"))
        {
        if (saving.length() > 10)
            {
            // do something
            }
        }
    }

or

if (saving == null) saving = "";

if (!saving.equals("true"))
    {
    if (saving.length() > 10)
        {
        // do something
        }
    }

Is is perhaps verbose but if you will use Java for programming it is the price to pay.

Using String literal equals() method to avoid NullPointerException has the following disadvantages

  1. the solution is tricky
  2. the code is not naturally readable for human
  3. equality will not crash but another part of program will continue to crash
  4. code become quickly difficult to read if this tips is used for lower() and greater() functions (example: if ('USA'.greater(sCode)))
  5. give a false impression that the code is safe

Upvotes: 3

oopexpert
oopexpert

Reputation: 755

Sorry but I cannot agree on that issue.

If you use the constant first you support structures that veil programming errors. And one should only produce code either

  1. to add functionality,
  2. fix an programming error
  3. or trying to support structures to avoid programming errors (like design patterns).

On top you have a weak assertion about the variable. Following code is always forced to check for null values as well.

If you have to deal with null values from other modules your task is to map them into proper representations in your domain model as early as possible.

In your own code avoid null as parameter or return value, so there won't be any null-checks neccessary anymore, the suggested piece of code included.

You see, this subject is more about avoiding null than accepting it to conquer your code.

Upvotes: 4

OldCurmudgeon
OldCurmudgeon

Reputation: 65851

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.

Upvotes: 30

Simon Dorociak
Simon Dorociak

Reputation: 33515

It's called yoda conditions (putting a constant before a variable in a comparison) can be considered like bad practise, maybe for someone much less readable (like me).

But sometimes using yoda condition makes the code more "comprehensible" -> you don't need to put extra null check in front of it and it efficiently differentiates block of code from case where null is strongly forbidden.

Upvotes: 8

user2864740
user2864740

Reputation: 61935

The format of putting the literal first avoids the potential NPE and will always be false for "literal".equals(null).

This is because a "literal" expression always evaluates to a String object (and is thus never null) and String.equals(obj) checks to see if the other object is a null (via asinstanceof). An object is an object - no need to worry bout the "heap".

It's good practice to have a null-guard in an equals implementation as null.equals(null) is not permissible: "The equals method implements an equivalence relation on non-null object references"

Here is the String.equals the source (for OpenJDK):

public boolean equals(Object anObject) {
    if (this == anObject) {
        return true;
    }
    if (anObject instanceof String) {
        // ..
    }
    return false;
}

Upvotes: 3

k4sia
k4sia

Reputation: 416

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

This is very safe and good practice. String "true" will never be null. So you will never compare your String to null. This piece of code is perfectly fine

Upvotes: 4

jakson
jakson

Reputation: 782

This is safe - and as you have seen, a good way of avoiding null pointers.

You mention the use of new for Strings. Many java static code analysis tools will recommend always using literals over new String("foo");.

Edit:

If you wanted, you could even just use: 

if (Boolean.valueOf(saving)) {
    ...
}

According to the docs, passing null will return false.

Upvotes: 25

Related Questions