Ternary operation in Java - isn't this abuse?

Question

I think the below code is difficult to understand. I also feel it abuses Java's ternary operator.

String name = ((this.getAllNamesAsDelimitedString().contains(incomingName) ? 
               incomingName:
               (CollectionsUtils.isEmptyCollection(this.getEntityOperationMap()) ? 
               null : 
               this.getEntityOperationMap.get(incomingName))));

Above getAllNamesAsDelimitedString will return a comma (,) delimited String.

I'd prefer something instead of such a nested ternary operator, if it looks cleaner and easier to understand. It would also be easy to sort out merge issue in, say, SVN.

N.B: There are even more nested ternary conditions in the code that I just have taken over.

Is there any general guideline which suggests the number of levels a ternary operator should be allowed? In other words, any guideline suggesting when you should break your ternary operator for better code readability?

Answer 1

Yes, that's abuse.

Generally speaking, I see two points in your code where you're inferring meaning based on the underlying data structures, which - if they were to change - would also need to change.

Additionally, these types of calls (getAllNamesAsDelimitedString().contains(incomingName) and (CollectionsUtils.isEmptyCollection(this.getEntityOperationMap()) typically end up getting used many times, which leads to DRY.

Additionally, (this.getAllNamesAsDelimitedString().contains(incomingName) violates law of demeter (it has two dots), and exposes what I'm assuming is an underlying collection of entities as a string, for no good reason.

Therefore, first up, I'd modify (this.getAllNamesAsDelimitedString().contains(incomingName) to this.containsName(incomingName)

Which makes the statement:

String name = ((containsName(incomingName)) ? incomingName:
               (CollectionsUtils.isEmptyCollection(this.getEntityOperationMap()) ? 
               null : 
               this.getEntityOperationMap.get(incomingName))));

Only a little better.

I'd apply the same refactoring to the next ternary, by introducing a hasOperations method:

 private boolean hasOperations()
 {
     return !CollectionUtils.isEmptyCollection(this.getEntityOperationMap())
 }

Which now makes the statement:

String name = ((containsName(incomingName)) ? incomingName:
               hasOperations() ? this.getEntityOperationMap.get(incomingName)) : null;

Again, a little better, but still unpalatable.

Finally, I'd swap this out for a real if..then

 private String getName(String incomingName)
 {
     if (containsName(incomingName))
        return incomingName;
     else 
        return hasOperations() ? getOperation(incomingName) : null;
 }

Answer 2

I think it's important to note that nested ternary operators can be formatted to look good:

String name = isConditionA ? "first"
            : isConditionB ? "second"
            : isConditionC ? "third"
                           : "fourth";

This isn't the most readable option in your case, because you have longer method calls, but I think it's better when the conditions are kept short and meaningful.

Answer 3

In my opinion I would go for more of code clarity than using cryptic Ternary operations. In cases where ternary operator statements become difficult to grasp at first glance, an if..else would be ideal. And moreover I am not aware of any performance difference in the application by using ternary operators. In you code example: It takes some time to figure out what's happening with the ternary operator, and even with correct indentation the code would not be as obvious as with an if...else.

May be you could benefit from the related discussion here.

Answer 4

I completely agree with user @seand — formatting makes all the difference in the world. I don't think your example is abuse at all, but I think it would be easier to follow with a few minor changes...

First, remove unnecessary parentheses:

String name = this.getAllNamesAsDelimitedString().contains(incomingName) ? 
              incomingName:
              CollectionsUtils.isEmptyCollection(this.getEntityOperationMap() ? 
              null : 
              this.getEntityOperationMap.get(incomingName);

Next, reformat the ternary expression tree as suggested by @seand:

String name = this.getAllNamesAsDelimitedString().contains(incomingName) ? incomingName
            : CollectionsUtils.isEmptyCollection(this.getEntityOperationMap() ? null
            : this.getEntityOperationMap.get(incomingName);

I often find it more readable to place the ? and the : just after their preceding character — with no intervening whitespace — as if they were English punctuation:

String name = isConditionA? "first":
              isConditionB? "second":
              isConditionC? "third":
                            "fourth";

which would give:

String name = this.getAllNamesAsDelimitedString().contains(incomingName)? incomingName:
              CollectionsUtils.isEmptyCollection(this.getEntityOperationMap()? null:
              this.getEntityOperationMap.get(incomingName);

But in this case, I would probably write it like this:

String name =
    this.getAllNamesAsDelimitedString().contains(incomingName)
        ? incomingName
        : CollectionsUtils.isEmptyCollection(this.getEntityOperationMap()
            ? null
            : this.getEntityOperationMap.get(incomingName);