Is it considered bad form to execute a function within a conditional statement?

Question

Consider a situation in which you need to call successive routines and stop as soon as one returns a value that could be evaluated as positive (true, object, 1, str(1)).

It's very tempting to do this:

if (fruit = getOrange())
elseif (fruit = getApple())
elseif (fruit = getMango())
else fruit = new Banana();

return fruit;

I like it, but this isn't a very recurrent style in what can be considered professional production code. One is likely to rather see more elaborate code like:

fruit = getOrange();
if(!fruit){
    fruit = getApple();
    if(!fruit){
        fruit = getMango();
        if(!fruit){
            fruit = new Banana();
        }
    }
}

return fruit;

According to the dogma on basic structures, is the previous form acceptable? Would you recommend it?

Edit:

I apologize to those who assumed that these functions were meant to be factories or constructors. They're not, they're just placeholders. The question is more about syntax than "factorying". These functions could as well be lambda.

Answer 1

The problem, as I see it, is not the structure, but the driving rules. Why does getOrange come before getApple, etc?

You are probably more likely to see something more data-driven:

enum FruitEnum
{
  Orange, Apple, Mango, Banana
}

and separately,

List<FruitEnum> orderedFruit = getOrderedFruit();
int i = 0;
FruitObj selectedFruit;
while(selectedFruit == null && i <= orderedFruit.Count)
{
    fruit = FruitFactory.Get(orderedFruit[i++]);
}
if(fruit == null)
{
    throw new FruitNotFoundException();
}

That said, to simplify your code, you can use a coalesce operator:

fruit = getOrange() ?? getApple() ?? getMango() ?? new Banana();

Answer 2

In a strongly-typed language that doesn't equate 0/null to false and non-0/non-null to true, I would say that it's probably safe, but marginally less readable in the general case, where your method names and number of parameters may be larger. I would personally avoid it, except for certain standard idioms, in cases where 0/null equate to false and non-0/non-null to true simply because of the potential danger of confusing assignment with equality checking in reading the code. Some idioms in weakly-typed languages, like C, are so pervasive that it doesn't make sense to avoid them, .e.g,

 while ((line = getline()) != null) {
   ...
 }

Answer 3

I can think of two alternatives.

The first is only allowable in languages like yours (PHP?), where single = in a conditional is ok.

    if ( (fruit = getOrange()) != null)
    elseif ( (fruit = getApple()) != null)
    elseif ( (fruit = getMango()) != null)
    else fruit = new Banana();

Makes it clear that you are doing a comparison and that the single = are not a mistake.

    fruit = getOrange();
    if(!fruit) fruit = getApple();
    if(!fruit) fruit = getMango();
    if(!fruit) fruit = new Banana();

Just like your second example, but gets rid of the ugly extra nesting.

Answer 4

If you want a succinct syntax, several languages allow using the "logical or" for this purpose (C# explicitly provides a coalescing operator, because nulls are not falsy).

Python:

fruit = ( getOrange() or 
          getApple()  or 
          getMango()  or 
          Banana() )

C#:

fruit = getOrange() ?? 
        getApple()  ?? 
        getMango()  ?? 
        new Banana();

Answer 5

To answer your question directly: it is usually bad form to have side-effects in conditional statement.

As a work around, you can store your fruit constructors in an array and find the first constructor which returns non-null (pseudocode):

let constructors = [getOrange; getApple; getMango; fun () -> new Banana()]
foreach constructor in constructors
    let fruit = constructor()
    if fruit != null
        return fruit

Its like a null-coalesce operator, but more generalized. In C#, you'd probably use linq as follows:

 var fruit = constructors
     .Select(constructor => constructor())
     .Filter(x => x != null)
     .First();

At least this way you can pass your constructors around like a factory class, instead of hard-coding them with the null-coalesce operator.

Answer 6

In C or C++, you could write:

return (fruit = getOrange()) ? fruit :
       (fruit = getApple())  ? fruit :
       (fruit = getMango())  ? fruit :
        new Banana();

The reason to avoid both this and your first version isn't "dogma on basic structures", it's that assignment on its own in a condition is confusing. Not all languages support it, for one thing. For another it's easily misread as ==, or the reader might be uncertain whether you really meant it, or perhaps intended ==. Adding != 0 to each condition gets quite dense and wordy.

GCC has an extension to allow:

return getOrange() ? : getApple() ? : getMango() ? : new Banana();

The same thing can often be achieved with || or or (but not in C or C++).

Another possibility is:

do {
    fruit = getOrange();
    if (fruit) break;
    fruit = getApple();
    if (fruit) break;
    fruit = getMango();
    if (fruit) break;
    fruit = new Banana();
} while (false);

This goes even better in a language where you can break out of a basic block, which you can with last in Perl, since you can dispense with the do / while(false). But probably only assembly programmers will actually like it.

Answer 7

I don't think anybody's mentioned yet that the first version can be kind of a pain to step through in a debugger. The difference in readability is debatable, but In general, I avoid assignments and function calls in conditionals to make it easier to trace through the execution, when necessary (even if I never need to debug it, someone else may need to modify the code later).