Ternary operation in Java - isn't this abuse?

Question

Locked. This question and its answers are locked because the question is off-topic but has historical significance. It is not currently accepting new answers or interactions.

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?

• Peter Taylor: As an aside, not directly related to the ternary operator, Map.get will return null if the map is empty, so the CollectionsUtils.isEmptyCollection branch is entirely unnecessary.
• Sulthan: @PeterTaylor This is probably a check if the map is null or empty. However, not the best of ideas to consider null maps the same as empty maps.
• Peter Taylor: @Sulthan, looking at it with fresh eyes, I think you're right. But it would be a lot more self-documenting if it actually compared it to null, since that's the only real purpose.
• Sulthan: @PeterTaylor Agreed.
• Mr_and_Mrs_D: this is a case where dropping this would help

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

• mprabhat: Nice one, very nice you mentioned 'law of demeter' :)
• RoToRa: One possibly to get rid of the final ternary operation could be to have getOperation return null when there are no operations. Or would that be too intransparent?
• Marty Pitt: @RoToRa It depends on what you believe the contract of getOperation() should be, and whether it's valid to return null if there are no operations, or to throw an exception. If it can return null, then - yes - it seems valid to move the ternary there.
• RoToRa: Considering that it would be strange to have getName return null if there are no operations, but throw an exception if there where operations and incomingName isn't one (the former is a special case of the latter) it's likely that getOperations would return null already (especially since it replaces this.getEntityOperationMap.get which does exactly that). Actually thinking about it, it's a bit strange the original code checks if the map is empty in the first place, because it's unnecessary.
• RoToRa: One more thing: Since working with Scala I'd be using an Option type/monad in cases like these thus explicitly declaring that it can return "nothing".

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.

• Todd Lehman: I agree completely. In fact, I think nested ternary operators are some of the most readable structures that exist!

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

• Martin Schröder: Nice. How do I get Eclipse's formatter to format the code that way?
• Martin Schröder: @MartinSchröder: See stackoverflow.com/q/8044946/821436