Calling methods inside if() - C#

Question

I have a couple of methods that return a bool depending on their success, is there anything wrong with calling those methods inside of the IF() ?

//&& makes sure that Method2() will only get called if Method1() returned true, use & to call both methods
if(Method1() && Method2())
{
    // do stuff if both methods returned TRUE
}

Method2() doesn't need to fire if Method1() returns FALSE.

Let me know there's any problem with the code above.

thank you.

EDIT: since there was nothing wrong with the code, I'll accept the most informative answer ... added the comment to solve the "newbie & &&" issue

Answer 1

Nothin' wrong.

Actually...I wouldn't name them Method1 and Method2. Something more descriptive. Maybe passive sounding too (like StuffHasHappened or DataHasLoaded)

Answer 2

No, nothing wrong with method calls in the if condition. Actually, that can be a great way to make your code more readable!

For instance, it's a lot cleaner to write:

private bool AllActive()
{
    return x.IsActive && y.IsActive && z.IsActive;
}

if(AllActive())
{
    //do stuff
}

than:

if(x.IsActive && y.IsActive && z.IsActive)
{
    //do stuff
}

Answer 3

As useful as they are, sequence points can be confusing. Unless you really understand that, it is not clear that Method2() might not get called at all. If on the other hand you needed BOTH methods to be called AND they had to return true, what would you write? You could go with

bool result1 = Method1();
bool result2 = Method2();
if (result1 && result2)
{
}

or you could go with

if (Method1())
    if (Method2())
    {
    }

So I guess the answer to you question IMHO is, no, it's not exactly clear what you mean even though the behavior will be what you describe.

Answer 4

Looks good to me, multiple clauses in the if() block will short circuit if an earlier condition fails.

Answer 5

There shouldn't be any problem.

The normal behavior is that Method1() will execute, and if that returns true Method2() will execute, and depending on what Method2() returns, you may / may not enter the if() statement.

Now, this assumes that the compiler generates code that executes that way. If you want to be absolutely sure that Method2() doesn't execute unless Method1() returns true you could write it like this

if( Method1() )
{
  if( Method2() )
  {
    // do stuff if both methods returned TRUE 
  }
}

But, I've always observed that your code will run as expected, so this is probably not necessary.

Answer 6

I'll throw in that you can use the & operator (as opposed to &&) to guarantee that both methods are called even if the left-hand side is false, if for some reason in the future you wish to avoid short-circuiting.

The inverse works for the | operator, where even if the left-hand condition evaluates to true, the right-hand condition will be evaluated as well.

Answer 7

I would only recommend it if the methods are pure (side-effect-free) functions.

Answer 8

While, as everyone says, there's nothing "wrong" with doing things this way, and in many cases you're doing precisely what the language was designed for.

Bear in mind, however, that for maintainabilities sake, if Method2 has side effects (that is, it changes somethings state) it may not be obvious that this function is not being called (a good programmer will usually know, but even good programmers sometimes have brain farts).

If the short circuited expression has some kind of side effect, it may be more readable to seperate the statements, strictly from a maintenance perspective.

Answer 9

Personally, I would consider

if(Method1() && Method2())
{
    // do stuff if both methods returned TRUE
}

to be a bad practice. Yes, it works in the current environment, but so does

if(Method1())
{
  if (Method2())
  {
    // do stuff if both methods returned TRUE
  }
}

But will it work in ALL environments? Will future, possibly non-Microsoft, C# compilers work this way? What if your next job involves another language where both methods will always be called? I wouldn't rely on that particular construct not because it's wrong, but because it doesn't solve any serious problem, and it may become wrong in the future

Answer 10

Looks good to me, but there are some caveats... This is NOT the kind of thing where blanket rules apply.

My guidelines are:

• If the method names are short, and there are not too many of them, then it's all good.
• If you have too many statements/method calls inside the if statement, you most likely are comparing more than one "set" of things. Break those "sets" out and introduce temporary variables.
• "Too many" is subjective, but usually more than around 3
• When I say "method names are short" I'm talking not just about the names, but the parameters they take as well. Basically the effort required for someone to read it. For example if( Open(host) ) is shorter than if( WeCouldConnectToTheServer ). The total size of all these items is what it comes down to.