46
votes
\$\begingroup\$

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?

\$\endgroup\$

locked by Jamal Aug 9 '16 at 22:18

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. See the help center for guidance on writing a good question.

Read more about locked posts here.

  • 3
    \$\begingroup\$ 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. \$\endgroup\$ – Peter Taylor Dec 6 '11 at 18:18
  • \$\begingroup\$ @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. \$\endgroup\$ – Sulthan May 4 '12 at 11:17
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – Peter Taylor May 4 '12 at 11:45
  • \$\begingroup\$ @PeterTaylor Agreed. \$\endgroup\$ – Sulthan May 4 '12 at 11:55
  • 1
    \$\begingroup\$ this is a case where dropping this would help \$\endgroup\$ – Mr_and_Mrs_D Jul 21 '13 at 12:17
48
votes
\$\begingroup\$

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;
 }
\$\endgroup\$
  • 1
    \$\begingroup\$ Nice one, very nice you mentioned 'law of demeter' :) \$\endgroup\$ – mprabhat Dec 5 '11 at 7:03
  • 2
    \$\begingroup\$ 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? \$\endgroup\$ – RoToRa Dec 5 '11 at 12:04
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – Marty Pitt Dec 6 '11 at 0:01
  • \$\begingroup\$ 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. \$\endgroup\$ – RoToRa Dec 6 '11 at 8:28
  • \$\begingroup\$ 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". \$\endgroup\$ – RoToRa Dec 6 '11 at 8:29
57
votes
\$\begingroup\$

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.

\$\endgroup\$
  • 5
    \$\begingroup\$ I agree completely. In fact, I think nested ternary operators are some of the most readable structures that exist! \$\endgroup\$ – Todd Lehman Jan 22 '12 at 7:52
6
votes
\$\begingroup\$

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.

\$\endgroup\$
3
votes
\$\begingroup\$

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);
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.