CODEWITHSUNDEEP
javavariablesnullternary-operator
Brandon Coffman
Brandon Coffman

Reputation: 244

Using ternary operator with 4 expressions

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;
}

Upvotes: 12

Views: 74924

Answers (8)

Svante
Svante

Reputation: 51501

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;};

EDIT (I am now more opinionated than 8 years ago)

Since you are looking for best practices:

// Use default visibility by default, especially in examples.
// Public needs a reason.
class MessageFormat {
    static final Color DEFAULT_COLOR = Color.RED;

    // Strongly prefer final fields.
    private final Color messageColor;

    // Protect parameters and variables against abuse by other Java developers
    MessageFormat (final Person person) {
        // Use Optionals; null is a code smell
        final Optional<Color> preferredColor = person.getPreferredColor();
        // Bask in the clarity of the message
        this.messageColor = preferredColor.orElse(DEFAULT_COLOR);
    }
}

Upvotes: 4

Alon
Alon

Reputation: 4952

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> ;
 }

Instead of a ternary syntax.

Upvotes: 1

Andreas Dolk
Andreas Dolk

Reputation: 114777

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 ;)

Upvotes: 1

brianegge
brianegge

Reputation: 29872

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.

Upvotes: 2

Christopher Gutteridge
Christopher Gutteridge

Reputation: 4535

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.

Upvotes: 25

Gabriel Reid
Gabriel Reid

Reputation: 2536

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.

Upvotes: 2

Mark Byers
Mark Byers

Reputation: 838256

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.

Upvotes: 1

djc
djc

Reputation: 11711

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.

Upvotes: 0

Related Questions