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

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 2

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 3

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 4

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.