Use of conditional operator to select which object calls a particular method?

Question

I have two collections, and items that are added to one or the other of those collections based on whether some criteria is met.

Somewhat inadvertantly, I stumbled on the fact that it is legal to write

(test(foo) ? cOne : cTheOther).add(foo);

instead of

if (test(foo)) {
 cOne.add(foo);
} else {
 cTheOther.add(foo);
}

While the first makes me feel clever (always a plus), I'm not sure about longer term readability, maintainability, etc. The basic advantage I see is that if I know I'm always going to be doing the same thing, it becomes one location to change a method (instead of two, or perhaps many if I'm implementing a switch statement via conditional operators). The main disadvantage occurs when that becomes not the case (i.e., I need add a method call to some cases and not others).

What pros and cons of the two methods (or other solutions) do you see?

If you don't think this particular instance of using the conditional operator to set which object to call a method with is the right choice, are there cases where it is reasonable?

Answer 1

As clever as the example is, readability and maintainability is always the smartest idea.

In regards to the conditional operator, you should only have a maximum of two values, if your condition goes beyond that maximum it may become unreadable. Conditional operators are not a standard way of dealing with different values.

Answer 2

IMHO the "full", long version is a little bit more readable and definitely better for maintenance in the long term.

Answer 3

A simpler code snippet would be

SomeClass component = test(foo) ? cOne : cTheOther;
component.add(foo);

In other words, separate the assignment from the use of it.

Answer 4

I would prefer the first one (Or the intermediate solution by matt). It is just shorter. If the conditional operator is used rather often in your project, people get used to it and "readability" increases.

Or in other words: if you are used to Perl, you can easily read it.

Answer 5

I am against the long-hand calling of the function in each branch of the if statement, because it's not always clear whether it's happenstance that the function to be called is the same and it's two places to change if you ever need to change the called function (okay, 'add' is an easy case). For that reason, I would normally assign the correct object to a variable (usually in an if statement, as there's usually a little other stuff going on too) but pull all common code outside the conditional.

Answer 6

Conditional operators used for this purpose is fairly rare so the eye isn't really looking for it.

I like my Java code to look consistent, so that me and future maintainers can easily spot things like branches, too much complexity, etc. Since I use ifs elsewhere, I can usually absorb the cost of doing it over using a conditional.

If I'm using a language where I'm doing lots of "tricks" via expressions, such as Python, that's a different story.

Answer 7

My preferred way is:

final boolean    result;
final Collection c;

result = test(foo);

if(result)
{
    c = cOne;;
} 
else 
{
    c = cOther;;
}

c.add(foo);

First off I don't like making calls and not assigning the value to temp variable (the test(foo) call) for two reasons:

1. it is easier to debug (System.out.println, or even the debugger - if the method has side effects then just looking at it in a debugger will call it again - thus causing the side effects).
2. even if there are no side effects, it discourages you from calling it more than once, which make the code more efficient. The compiler/hotspot should deal with the removal of the unneeded temp variable.

Secondly I don't like having code where, if you blur your eyes, it looks the same. Having cOne.add(foo) and cOther.add(foo) "look" the same if you "blur" your eyes. If, for example, you were changing it to a List and using add(int, E) instead of add(E) then you only have one place to change the code, which means less change of making a mistake (like cOne.add(1, foo) and cOther.add(2, foo) when they should both be add(1, foo)).

EDIT (based on the comment)

There are a couple of choices, it depends on how you have the code layed out. I would probably go with something like:

private Collection<Whatever> chooseCollection(final Whatever             foo,
                                              final Collection<Whatever> a,
                                              final Collection<Whatever> b)
{
    final boolean              result;
    final Collection<Whatever> c;

    result = test(foo);

    // could use a conditional - I just hate using them
    if(result)
    {
        c = a;
    }
    else
    {
        c = b;
    }

    return (c);
}

and then have something like:

for(......)
{
    final Collection<Whatever> c;
    final Whatever             foo;

    foo = ...;
    c = chooseCollection(foo, cOne, cOther);
    c.add(foo;
}

Essentially I make a method for anything inside of a { } block if it makes sense (and usually it does). I like to have a lot of small methods.

Answer 8

Adding to what matt b said, you could go one step further and use this format, when you decide to use the conditional operator:

SomeClass component = test(foo) 
   ? cOne 
   : cTheOther;
component.add(foo);

It gives the code more of an if-else feel, and makes it easier to read.

Answer 9

One potential problem with using conditional operators is when the arguments start to become more complex. For example, if the following line throws a NullPointerException:

String aString = obj1.getValue() == null ? obj2.getString() : obj1.getValue().getString();

which of the the three dereferences is causing the NPE? obj1, obj2 or obj1.getValue() Sometimes it might be possible through inference in the preceding code to work it out, but not always…

That said, compare the following two pieces of code:

final String aString = obj1 == null ? "default" : obj1.getString();

vs

final String aString;
if (obj1 == null) {
  aString = "default";
} else {
  aString = obj1.getString();
}

Most people would argue that the conditional is simpler to read, and it is certainly more concise.

Now, with that out of the way, would I use a conditional to call a method? No, it adds a lot of visual complexity, and one has to remember, that debugging is twice as hard as writing code. That means that the simpler the code is, the simpler it is for you, or someone else, to understand it in six months time. I would be more than happy to use the two line version in the accepted answer.