CODEWITHSUNDEEP
c#readability
Greg D
Greg D

Reputation: 44086

Which code is more readable?

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.

Upvotes: 32

Views: 3914

Answers (21)

Davit Siradeghyan
Davit Siradeghyan

Reputation: 6323

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

Upvotes: 0

unknown
unknown

Reputation:

The first one, it's much more debuggable

Upvotes: 3

Kena
Kena

Reputation: 6921

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.

Upvotes: 0

Gabe Moothart
Gabe Moothart

Reputation: 32082

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.

Upvotes: -1

please delete me
please delete me

Reputation:

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

Upvotes: 1

aib
aib

Reputation: 46971

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.

Upvotes: 9

Jon Skeet
Jon Skeet

Reputation: 1501163

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.

Upvotes: 6

terjetyl
terjetyl

Reputation: 9565

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.

Upvotes: -3

Eric Lippert
Eric Lippert

Reputation: 660169

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.

Upvotes: 93

vgru
vgru

Reputation: 51244

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.

Upvotes: 1

Jeff Hornby
Jeff Hornby

Reputation: 13650

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.

Upvotes: 1

David
David

Reputation: 73564

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.

Upvotes: 1

BenAlabaster
BenAlabaster

Reputation: 39846

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.

Upvotes: 3

Konamiman
Konamiman

Reputation: 50283

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.

Upvotes: 1

itsmatt
itsmatt

Reputation: 31416

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.

Upvotes: 0

AndreyAkinshin
AndreyAkinshin

Reputation: 19021

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.

Upvotes: 1

SLaks
SLaks

Reputation: 887547

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 &&.

Upvotes: 13

noctonura
noctonura

Reputation: 13121

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.

Upvotes: 19

leppie
leppie

Reputation: 117260

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

Upvotes: 4

Matt Huggins
Matt Huggins

Reputation: 83289

I like this shorthand notation assuming your language permits it:

SomeProperty = Foo() ? Bar() : false;

Upvotes: 46

JaredPar
JaredPar

Reputation: 754943

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.

Upvotes: 5

Related Questions