32

Suppose I have two methods bool Foo() and bool Bar(). Which of the following is more readable?

if(Foo())
{
    SomeProperty = Bar();
}
else
{
    SomeProperty = false;
}

or

SomeProperty = Foo() && Bar();

On the one hand, I consider the short-circuiting && to be a useful feature and the second code sample is much shorter. On the other hand, I'm not sure people are generally accustomed to seeing && outside a conditional statement, so I wonder if that would introduce some cognitive dissonance that makes the first sample the better option.

What do you think? Are there other factors that affect the decision? Like, if the && expression is longer than one line that can fit on the screen, should I prefer the former?

Post-answer clarifications:

A few things that I should have included in the initial question that the answers brought up.

  • Bar() may be more expensive to execute than Foo(), but neither method should have side effects.
  • The methods are both named more appropriately, not like in this example. Foo() boils down to something like CurrentUserAllowedToDoX() and Bar() is more like, XCanBeDone()
Greg D
  • 43,259
  • 14
  • 84
  • 117

21 Answers21

93

I agree with the general consensus that the Foo() && Bar() form is reasonable unless it is the case that Bar() is useful for its side effects as well as its value.

If it is the case that Bar() is useful for its side effects as well as it's value, my first choice would be to redesign Bar() so that production of its side effects and computation of its value were separate methods.

If for some reason that was impossible, then I would greatly prefer the original version. To me the original version more clearly emphasizes that the call to Bar() is part of a statement that is useful for its side effects. The latter form to me emphasizes that Bar() is useful for its value.

For example, given the choice between

if (NetworkAvailable())
  success = LogUserOn();
else
  success = false;

and

success = NetworkAvailable() && LogUserOn();

I would take the former; to me, it is too easy to overlook the important side effect in the latter.

However, if it were a choice between

if (NetworkAvailable())
  tryWritingToNetworkStorage = UserHasAvailableDiskQuota();
else
  tryWritingToNetworkStorage = false;

and

tryWritingToNetworkStorage = NetworkAvailable() && UserHasAvailableDiskQuota();

I'd choose the latter.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • 3
    (Marked as answer because this captures the relevant design details and considerations req'd to make the determination in the general case) – Greg D Oct 09 '09 at 17:20
  • It should also be noted that if neither Foo nor Bar have side effects, and if running Bar without an error doesn't depend on Foo being true, you should put the most performant method first. In other words, if Foo makes a call over the network and Bar simply looks at an in-memory variable, then `SomeProperty = Bar() && Foo()` would be preferred. – StriplingWarrior Oct 09 '09 at 17:29
  • 4
    I wonder if Linus Torvalds has an account here... I want to see him answering C questions! – Sarah Vessels Oct 09 '09 at 17:43
  • In this case, `Bar()` does not (certainly should not) have side-effects, so I've opted for `Foo() && Bar()`. – Greg D Oct 09 '09 at 17:43
  • – Greg D Oct 09 '09 at 17:51
  • 2
    No..., what was the joke? Why is Eric saying "We're just guys, you know?" :) – Joan Venge Oct 09 '09 at 17:52
  • 3
    @Joan: I was gushing about how both Eric and Jon Skeet answering my question made me want to take a cold shower. Like I said, bad. :) – Greg D Oct 09 '09 at 17:55
  • @StriplingWarrior Depends. For example, assume Foo() takes 3 seconds and returns `false` 90% of the time, Bar() takes 1 second and returns `false` 10% of the time. On average, `Foo() && Bar()` takes (0.9*3 + 0.1*(3+1))/2 = 1.55 seconds, whereas `Bar() && Foo()` takes (0.1*1 + 0.9*(1+3))/2 = 1.85 seconds. Despite Foo() being less performant, the optimal solution is `Foo() && Bar()`. Exact measurements and probabilities may not always be available, of course, in which case you could guess, go with the result of some form of tests, or simply default to your method. – Matthew Read Jul 07 '11 at 00:28
46

I like this shorthand notation assuming your language permits it:

SomeProperty = Foo() ? Bar() : false;
Matt Huggins
  • 81,398
  • 36
  • 149
  • 218
  • 19
    I don't understand how this could be more readable then "SomeProperty = Foo() && Bar();" THere is no difference in length and && is so much more common (and therefore understandable). – noctonura Oct 09 '09 at 16:47
  • 1
    In case many function we have bad code like this: SomeProperty = Foo1() ? (Foo2() ? Foo3() : false) : false; But if we use && then we have more readable code: SomeProperty = Foo1() && Foo2() && Foo3(); – AndreyAkinshin Oct 09 '09 at 16:51
  • 13
    The conditional operator has been a mainstay of C based languages for a long time. It is perfectly clear and IMHO the better choice here. – Sinan Ünür Oct 09 '09 at 16:52
  • 13
    This is much more readable than the case with &&. It is a more general notion since it would work with other functions than ones returning boolean values. – Kasper Holdum Oct 09 '09 at 16:52
  • 6
    I prefer this one. Maybe I'm just used to using the ternary operator, so this is more clear to me. – Jason Down Oct 09 '09 at 16:53
  • 6
    I prefer the ternary operator, as well. I find it to be much more readable, and - at least for me - my mind reads them as if they were conditional statements. The fact that there is a question as to && not being as readable, I would go with the more common one. @Rich, while short-circuit operators are common, I don't think that using them in assignments is as common as using them in conditions. Not basing that just upon my own code, but from what I have seen in others code, stuff from Codeplex, etc. – Joseph Ferris Oct 09 '09 at 16:58
  • 1
    This is a good answer, and I didn't supply enough context in the question to make it wrong, but as my comment in marked answer says, after thinking about it a couple more minutes, the core concept is the conditional relation between the two results, not the execution of the second result being conditional on the execution of the first. – Greg D Oct 09 '09 at 17:02
  • 1
    This is more explicit in what is happening, and a valid option. It could be the right choice, but Foo() and Bar() don't help make the decision clearer. See SLaks answer, meaningful method names are key. – NerdFury Oct 09 '09 at 17:03
19
SomeProperty = Foo() && Bar();

This is way more readable. I don't see how anyone could choose the other one frankly. If the && expression is longer then 1 line, split it into 2 lines...

SomeProperty = 
   Foo() && Bar();

      -or-

SomeProperty = Foo() && 
               Bar();

That barely hurts readability and understanding IMHO.

noctonura
  • 12,763
  • 10
  • 52
  • 85
  • 1
    I think this captures the essence. The names of the methods are a bit more descriptive (`IsAllowedToX` and `CanDoX`, more or less), so I believe the boolean operator makes sense in context with the method names. The short circuiting aspect isn't as imperative as the combined cond'l. (We could check CanDoX first, but it will tend to be much slower than IsAllowedToX) – Greg D Oct 09 '09 at 16:59
13

It depends on what Foo and Bar do.

For example, IsUserLoggedIn() && IsUserAdmin() would definitely be better as an &&, but there are some other combinations (I can't think of any offhand) where the ifs would be better.

Overall, I would recommend &&.

SLaks
  • 868,454
  • 176
  • 1,908
  • 1,964
  • 7
    +1 for bringing up self documenting code. If the method names aren't clear in the behavior, the boolean operation won't be either. – NerdFury Oct 09 '09 at 16:59
9

Neither. I'd start with renaming SomeProperty, Foo and Bar.

What I mean is, you should structure your code as to convey your intentions clearly. With different functions, I might use different forms. As it stands, however, either form is fine. Consider:

IsFather = IsParent() && IsMale();

and

if (FPUAvailable()) {
    SomeProperty = LengthyFPUOperation();
} else {
    SomeProperty = false;
}

Here, the first form stresses the logical-and relationship. The second one stresses the short-circuit. I would never write the first example in the second form. I would probably prefer the second form for the second example, especially if I was aiming for clarity.

Point is, it's hard to answer this question with SomeProperty and Foo() and Bar(). There are some good, generic answers defending && for the usual cases, but I would never completely rule out the second form.

aib
  • 45,516
  • 10
  • 73
  • 79
6

In what way do you think people might misinterpret the second one? Do you think they'll forget that && shortcircuits, and worry about what will happen if the second condition is called when the first is false? If so, I wouldn't worry about that - they'd be equally confused by:

if (x != null && x.StartsWith("foo"))

which I don't think many people would rewrite as the first form.

Basically, I'd go with the second. With a suitably descriptive variable name (and conditions) it should be absolutely fine.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 1
    Not necessarily misinterpret so much as require an extra pass just because they don't see && in a statement like this as often as in a cond'l expression. At my workplace, except in code that I and a very select few others have written (like, 2 people), I've never seen the && used in this way. – Greg D Oct 09 '09 at 16:52
  • 7
    I would say this would be a good time to educate them then - improve the lowest common denominator instead of pandering to it :) – Jon Skeet Oct 09 '09 at 17:38
5

In the case where the conditional expressions are short and succinct, as is this case, I find the second to be much more readable. It's very clear at a glance what you are trying to do here. The first example though took me 2 passes to understand what was happening.

JaredPar
  • 733,204
  • 149
  • 1,241
  • 1,454
4

I would go for the second, but would probably write it like the first time, and then I would change it :)

leppie
  • 115,091
  • 17
  • 196
  • 297
  • Hehe, that's what I did leppie. Writing it out, I did it the first way. Now I'm on my review pass of the stuff I just wrote and thought, "Humm, this might be way better this other way instead." – Greg D Oct 09 '09 at 16:46
3

When I read the first one, it wasn't immediately obvious SomeProperty was a boolean property, nor that Bar() returned a boolean value until you read the else part of the statement.

My personal view is that this approach should be a best practice: Every line of code should be as interpretable as it can with reference to as little other code as possible.

Because statement one requires me to reference the else part of the statement to interpolate that both SomeProperty and Bar() are boolean in nature, I would use the second.

In the second, it is immediately obvious in a single line of code all of the following facts:

  • SomeProperty is boolean.
  • Foo() and Bar() both return values that can be interpreted as boolean.
  • SomeProperty should be set to false unless both Foo() and Bar() are interpreted as true.
BenAlabaster
  • 39,070
  • 21
  • 110
  • 151
3

The first one, it's much more debuggable

  • Not if you're using a good debugger ... are there even any commonly used debuggers that don't allow debugging into two separate functions used in the same statement? I think Jon Skeet's answer applies here -- his `if` statement is not hard to debug either. – Matthew Read Jul 07 '11 at 00:33
1

I think, that best way is use SomeProperty = Foo() && Bar(), because it is much shorter. I think that any normal .NET-programmer should know how &&-operator works.

AndreyAkinshin
  • 18,603
  • 29
  • 96
  • 155
  • I thought that too, until I read this question and some of its answers: http://stackoverflow.com/questions/836945/i-dont-like-this-is-this-cheating-the-language – Greg D Dec 05 '09 at 17:18
1

I would choose the long version because it is clear at first glance what it does. In the second version, you have to stop for a few secons until you realize the short-circuit behavior of the && operator. It is clever code, but not readable code.

Konamiman
  • 49,681
  • 17
  • 108
  • 138
1

Wait a minute. These statements aren't even equivalent, are they? I've looked at them several times and they don't evaluate to the same thing.

The shorthand for the first version would be using the ternary operator, not the "and" operator.

SomeProperty = foo() ? bar: false

However, logic error aside, I agree with everyone else that the shorter version is more readable.

Edit - Added

Then again, if I'm wrong and there IS no logic error, then the second is way more readable because it's very obvious what the code is doing.

Edit again - added more:

Now I see it. There's no logic error. However, I think this makes the point that the longer version is clearer for those of us who haven't had our coffee yet.

David
  • 72,686
  • 18
  • 132
  • 173
  • 1
    I can't argue with the coffee comment, especially with the sample method names I provided. :) It might make more sense if I had given the real method names. – Greg D Oct 09 '09 at 17:08
1

If, as you indicate, Bar() has side effects, I would prefer the more explicit way. Otherwise some people might not realize that you are intending to take advantage of short circuiting.

If Bar() is self contained then I would go for the more succinct way of expressing the code.

Jeff Hornby
  • 12,948
  • 4
  • 40
  • 61
1

If the first version looked like this:

if (Foo() && Bar())
{
    SomeProperty = true;
}
else
{
    SomeProperty = false;
}

or like this:

if (Foo())
{
    if (Bar())
       SomeProperty = true;
    else
       SomeProperty = false;
}
else
{
    SomeProperty = false;
}

Then it would at least be consistent. This way the logic is much harder to follow than the second case.

vgru
  • 49,838
  • 16
  • 120
  • 201
1

It is often useful to be able to breakpoint any specific function call individually, and also any specific setting of a variable to a particular value -- so, perhaps controversially, I would be inclined to go for the first option. This stands a greater chance of allowing fine-grained breakpointing across all debuggers (which in the case of C# is not a very large number).

That way, a breakpoint may be set before the call to Foo(), before the call to Bar() (and the set of the variable), and when the variable is set to false -- and obviously the same breakpoints could also be used to trap the Foo() vs !Foo() cases, depending on the question one has in mind.

This is not impossible to do when it is all on one line, but it takes attention that could be used to work out the cause of whatever problem is being investigated.

(C# compiles quickly, so it is usually not a big problem to rearrange the code, but it is a distraction.)

0

It comes down to being intentional and clear, in my mind.

The first way makes it clear to the casual observer that you aren't executing Bar() unless Foo() returns true. I get that the short circuit logic will keep Bar() from being called in the second example (and I might write it that way myself) but the first way is far more intentional at first glance.

itsmatt
  • 31,265
  • 10
  • 100
  • 164
0

Short-circuiting of AND behavior is not standard in all languages, so I tend to be wary of using it implicitly if that's essential to my code.

I don't trust myself to see the short-circuit immediately after I've switched languages for the fifth time in the day.

So if Boo() should never be called when Foo() returns false, I'd go with version #2, if only as a defensive programming technique.

Kena
  • 6,891
  • 5
  • 35
  • 46
0

First method is more readable, but get more lines of codes, second is more EFFECTIVE, no comparision, only one line! I think that second is better

Davit Siradeghyan
  • 6,053
  • 6
  • 24
  • 29
-1

Since you're using c#, I would choose the first version or use the ternary ?: operator. Using && in that manner isn't a common idiom in c#, so you're more likely to be misunderstood. In some other languages (Ruby comes to mind) the && pattern is more popular, and I would say it's appropriate in that context.

Gabe Moothart
  • 31,211
  • 14
  • 77
  • 99
-3

I would say code is readable if it is readable to a person with no knowledge of the actual programming language so therefore code like

if(Foo() == true and Bar() == true)
then
    SomeProperty = true;
else
    SomeProperty = false;

is much more readable than 

SomeProperty = Foo() && Bar();

If the programmer taking over the code after you isn't familiar with the latter syntax it will cost him 10 times the time it takes you to write those extra few characters.

terjetyl
  • 9,497
  • 4
  • 54
  • 72