30
votes
\$\begingroup\$

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?

\$\endgroup\$

locked by Jamal Jan 8 '14 at 5:35

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.

  • \$\begingroup\$ Does totalOpenInvoices represent the sum of the monetary amount of the open invoices, or is it just the number of invoices that are open? \$\endgroup\$ – Dr. Wily's Apprentice Dec 22 '11 at 19:13
  • \$\begingroup\$ it's the sum of the amounts from every open invoice \$\endgroup\$ – Aaron Anodide Dec 22 '11 at 19:30
  • 37
    \$\begingroup\$ Your rewrite is a lot harder to read, IMO. \$\endgroup\$ – Ben Brocka Dec 22 '11 at 20:13
  • 16
    \$\begingroup\$ this is a case of "I've just learned some really cool stuff, let me try it out on the first thing that comes my way". Been there. Done it. Not worth it. \$\endgroup\$ – siamii Dec 22 '11 at 21:51
  • 2
    \$\begingroup\$ this syntax is odd to people who are not familiar with functional programming. However, it might be worth it when you have more than 2 terms to check. also extracting the lambda expression into a named property might make it more readable and please people who prefer a fluent style. private Func<int, bool> IsNotZero { get { return c => c != 0; } } ... if (new[] { budgetRemaining, totalOpenInvoices, otherThings, moreStuff, etc, ... }.Any(IsNotZero)) { } \$\endgroup\$ – Tion Dec 22 '11 at 22:45
48
votes
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ simple-talk.com/dotnet/performance/… The object would not be "garbage collected" unless it was still in use at time of collection. That is, if it wasn't in use, it would not get moved to Gen1, it would just be wiped. \$\endgroup\$ – taylonr Dec 22 '11 at 17:47
  • 4
    \$\begingroup\$ Other way around: it would be collected if NOT still in use. If it WAS still in use, it would move to Gen1. But the point is, there's an array created and not referenced anywhere else but the condition if the if statement so it would be instantly eligible for collection. \$\endgroup\$ – Jesse C. Slicer Dec 22 '11 at 18:05
  • 2
    \$\begingroup\$ Using the Any() method here is overkill in this case. It should be reserved for the longer cases where there are maybe 4 or more separate conditions or if you just happen to have the collection lying around. \$\endgroup\$ – Jeff Mercado Dec 22 '11 at 19:40
  • 6
    \$\begingroup\$ GM called, and they don't like you calling their Buicks fly swatters. :-) \$\endgroup\$ – LarsTech Dec 22 '11 at 21:55
29
votes
\$\begingroup\$

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.

\$\endgroup\$
  • 3
    \$\begingroup\$ True. Solutions should be as simple as possible (but no simpler). \$\endgroup\$ – George Duckett Dec 23 '11 at 11:56
22
votes
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ I would prefer to replace the local variable needName with a private function needName() \$\endgroup\$ – Dorus Dec 25 '11 at 13:08
8
votes
\$\begingroup\$

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
\$\endgroup\$
5
votes
\$\begingroup\$

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?

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

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. :)

\$\endgroup\$
1
vote
\$\begingroup\$

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:)

\$\endgroup\$

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