Using the ternary operator to determine a variable is not null and assigning it to another

Question

Is this an acceptable coding practice?

public class MessageFormat {
    private static final Color DEFAULT_COLOR = Color.RED;

    private Color messageColor = DEFAULT_COLOR;

    public MessageFormat(Person person) {
        Color color = person.getPreferredColor();
        messageColor = (color != null) ? color : messageColor; // this line
    }
}

or am I better off going with the classic ...

if (color != null) {
    messageColor = color;
}

Answer 1

Seems fine to me (I use Python's ternary operator a lot), but this kind of style issue is usually highly subjective. If the project has a coding style document, you may want to check that.

Answer 2

I prefer the second because it is more clearly expressing what you mean: you only want to change the color if it is not null. The first method doesn't make this so clear.

Answer 3

Use of the ternary operator is often a sensitive issue, along with other coding standards. It's use is probably best determined by coding standards at your site.

However, in this specific situation I would definitely recommend the second option; not only is it more clear, but the use of the ternary operator is totally unnecessary here. There's no need to re-assign messageColor to itself, so the only function of the ternary operator in this particular situation is code obfuscation.

Answer 4

Ternary operators often get abused as the code they produce seems clever and compact. In fact they make the code less readable and more error prone. whenever possible it is advisable to use the longer version of

if ( <condition>  {
    <action> ;
}

Answer 5

The ternary operator is more common among C programmers. In C if you avoid control structures you can often getting better pipelining, as there is no branch prediction to go wrong. I doubt you would see any performance difference in Java, and the if-null-then-assign pattern is far more common than the ternary. However, if you are maintaining a existing codebase, it's usually best to stay consistent with the existing code.

If you find yourself doing this a lot, you can write a defaultIfNull, firstNonNull or coalesce function which may make the code even more concise. Apache Commons Lang includes a defaultIfNull function.

Some languages include a ||= operator, which is the usual idiom for defaulting values in those languages.

Answer 6

Use of the ?: operator should be confined to make code more readable. A classic example:

a = sprintf( "There are %i green bottle%s on the wall.", i, (i==1?"":"s") );

In this case the code would be less readable if you broke it up into about 5 if/else lines.

I generally put brackets around the entire operator so that when reading it I mentally parse it as a single value.

 messageColor = (color != null ? color : messageColor);

Another variant is

messageColor = color || messageColor;

Which in some languages will evaluate to "color, unless color evaluates to "false", in which case value of messageColor. In my opinion this should be avoided as it may confuse people.

The most important thing is to be consistent so the next person reading your code (even if it's you) has minimum cognitive overhead.

Answer 7

Note that Java 7 (some way off, admittedly) will provide the Elvis Operator ?:

The ?: binary "Elvis" operator results in the value of the left-hand-side if it is not null, avoiding evaluation of the right-hand-side. If the left-hand-side is null, the right-hand-side is evaluated and is the result.

Obviously you can't use this yet (perhaps the OpenJDK 7 supports this?), but it's worth being aware of this.

Answer 8

In your case, I'd prefer the 'classic' implementation, because to me it's faster to understand, that you only want to use a new color if the person has a preferred one.

I sometimes use it in method calls if I want to avoid NPEs, but I usually elimate those ugly pieces of code in one of the next refactorings ;)

Answer 9

Readability, ease of understanding etc. are the same in this case (I mean, come on...). I don't like the duplication and apparent self assignment in the first example; it would translate to something like:

if (colour != null) {messageColour = colour;}
   else {messageColour = messageColour;};

which is a bit stupid.

I would usually write the second in one line, but that's a question of individual fancy resp. coding style guidelines:

if (colour != null) {messageColour = colour;};