Are these if-statements too fancy?

Question

I just rewrote this:

if (budgetRemaining != 0 || totalOpenInvoices != 0)
{
}

Like this:

if (new[] { budgetRemaining, totalOpenInvoices }.Any(c => c != 0))
{
}

If I had seen that before I ramped up on Linq, it would have confused me. Now that I've been learning functional programming and using Linq, it seems natural, but is it sacrificing simplicity?

Answer 1

Seems to be swatting a fly with a Buick to me. The first form seems pretty concise and the variable names are quite descriptive. The second form creates a new object (the array) which will eventually have to be GC'd and introduces a new lambda variable, c which doesn't seem descriptive any more.

Answer 2

Fancy is not the word for it. This is insane. Ever heard of the KISS principle? Keep-it-simple-stupid. Unless of course you are intentionally looking for ways to make your code look obfuscated, and your executable file bloated.

Answer 3

As a Java developer, the first is much easier to read. It could be confusing too: why the author use LINQ instead of a simple ||? It reminds me the law of the instrument: "if all you have is a hammer, everything looks like a nail".

Anyway, maybe it's worth creating a well-named local variable for the condition which could help the readers:

boolean needName = budgetRemaining != 0 || totalOpenInvoices != 0
if (needName) {
    ...
}

Creating a named local variable would improve the readability of the second version too.

Answer 4

In C# you have some syntactic noise, which makes this code even less readable, overweighing the benefits of such a low-yielding abstraction by a large margin. Even in a language like Haskell, famous for its abstraction power, the best you can do is this:

any (/= 0) [budgetRemaining, totalOpenInvoices]

Which is, although usable, still longer than

budgetRemaining /= 0 || totalOpenInvoices /= 0

Answer 5

If possible, I might prefer to do something like this:

IEnumerable<decimal> GetBalances()
{
    decimal budgetRemaining = 0M;

    // calculate budget remaining

    yield return budgetRemaining;

    decimal totalOpenInvoices = 0M;

    // calculate the total of the open invoices

    yield return totalOpenInvoices;

    // ... calculate and yield any other balances that require consideration
}

and then I could write the if statement like this:

    if (this.GetBalances().Any(balance => balance != 0M))
    {
    }

Note: I doubt that I would use this approach if I only needed to check 2 balances, but I might do this if I had very many different balances that needed to be checked.

Original Answer

EDIT: The comment on the question indicates that the totalOpenInvoices variable represents the total monetary amount of the open invoices, therefore this answer doesn't really apply to the question. I'll leave it here, though, as I feel that the argument would still be valid for the situation described in this answer.

Does totalOpenInvoices represent the sum of the monetary amount of the open invoices, or is it just the number of invoices that are open?

If the latter, then one reason why I don't like this approach is because you're sort of comparing apples and oranges. You're saying that the check budgetRemaining != 0 is similar in nature to the check totalOpenInvoices != 0, and the only difference is the input variable (either budgetRemaining or totalOpenInvoices).

I'll try illustrate why this doesn't make sense to me by breaking the if statement down in a different way. I'll move the != 0 code out into a delegate called stillHasMoney:

Func<decimal, bool> stillHasMoney = (amount) => amount != 0;

if (stillHasMoney(budgetRemaining) || stillHasMoney(totalOpenInvoices))
{
}

Now, does this code still make sense? If totalOpenInvoices represents the number of open invoices, not the sum of the monetary amount of the open invoices, then this code doesn't seem correct to me. It's like saying "I had 5 turtles and gave away 2. How much money is left?" Similarly, this code is saying "I had 5 open invoices and closed 2. How much money is left?" Granted, invoices translate to money more easily than turtles, but the point is, do you care about the number of open invoices or the sum of their monetary value?

Answer 6

Assuming both budgetRemaining and totalOpenInvoices are guaranteed to be non-negative, you could write:

if (budgetRemaining + totalOpenInvoices > 0)
{
}

Of course, while that simplifies the code, it sacrifices clarity and maintainability. So don't do that. :)

Answer 7

Sometimes it's just handier to have a function like

bool IsAlive(..)
{
 return (budgetRemaining != 0 || totalOpenInvoices != 0);
}

it doesn't directly apply to your question, just a side note:)