Checking if a method returns false: assign result to temporary variable, or put method invocation directly in conditional?

Question

Is it a good practice to call a method that returns true or false values in an if statement?

Something like this:

private void VerifyAccount()
{
    if (!ValidateCredentials(txtUser.Text, txtPassword.Text))
    {
        MessageBox.Show("Invalid user name or password");
    }
}

private bool ValidateCredentials(string userName, string password)
{
    string existingPassword = GetUserPassword(userName);
    if (existingPassword == null)
        return false;

    var hasher = new Hasher { SaltSize = 16 };
    bool passwordsMatch = hasher.CompareStringToHash(password, existingPassword);

    return passwordsMatch;
}

or is it better to store them in a variable then compare them using if else values like this

bool validate = ValidateCredentials(txtUser.Text, txtPassword.Text);
if(validate == false){
    //Do something
}

I am not only referring to .NET, I am referring to the question in all programming languages it just so happens that I used .NET as an example

Answer 1

As with all these things it depends.

If you aren't going to use the result of your call to ValidateCredentials then there's no need (other than for debugging purposes) to store the result in a local variable. However, if it makes the code more readable (and hence more maintainable) to have a variable go with that.

The code isn't going to be measurably less efficient.

Answer 2

Why use an additional variable? I prefer use the first approach, it's more readable and simple.

Answer 3

Is it a good practice to call a method that returns true or false values in an if statement?

Yes, if the conditional is not simple enough to inline and have it be readable.

or is it better to store them in a variable then compare them using if else values like this

You should only do this if your using the value in multiple places or need it to make the code more readable. Otherwise the assignment to a variable is unnecessary. Unnecessary code is at best wasteful and at worst a source of a defect.

Answer 4

Let see...

Because it's all about KISS, there's no need to create an additional variable when you can do without it. Also, there's no need to type more... when there's no need.

But then because you DRY, if you were later calling ValidateCredentials and find yourself typing ValidateCredentials(txtUser.Text, txtPassword.Text) you know you should have created an additional variable.

Answer 5

Yes, it is usually fine to use such methods as if conditions. It is helpful though if the method name indicates that the method returns a bool; for instance CanValidateCredentials. In C-style languages this method is often takes the form of Is and Can prefixes, and in Ruby with the '?' suffix.

Answer 6

There's another concern that has not yet been pointed out: short-circuit evaluation. What's the difference between these two code fragments?

Ex #1:

if(foo() && bar() && baz())
{
    quz();
}

Ex #2:

bool isFoo = foo();
bool isBar = bar();
bool isBaz = baz();
if(isFoo && isBar && isBaz)
{
    quz();
}

These two fragments of code appear to do the same thing, but if your language supports short circuit evaluation, these two fragments are different. Short-circuit evaluation means that the code will evaluate the minimum it needs to pass or fail a condition. If example #1, if foo() returns false, bar() and baz() are not even evaluated. This is useful if baz() is a long-running function call because you can skip it if either foo() returns false or foo() returns true and bar() returns false.

This is not the case in example #2. foo(), bar(), and baz() are always evaluated. If you are expecting bar() and baz() to exhibit side effects, you'll have problems with Example #1. Example #1 above is actually equivalent to this:

Ex #3:

if(foo())
{
    if(bar())
    {
        if(baz())
        {
            quz();
        }
    }
}

Be aware of these differences in your code when choosing between examples #1 and #2.

Answer 7

Expanding a bit on the readability issue ...

I can think of two good reasons to store the result in a variable:

1. If you're going to be using the condition more than once, saving it in a variable means you only have to call the function once. (This assumes that the stored value is still valid; if you need to re-test it, of course this doesn't apply.)

2. If storing it in a variable improves readability by giving the condition a name that's more meaningful than what you see in the function call.

For example, this:

bool foo_is_ok = is_ok(foo);
if (foo_is_ok) ...

doesn't help readability, but this:

bool done_processing = feof(file1) && feof(file2);
if (done_processing) ...

probably does, since it's not immediately obvious that feof(file1) && feof(file2) means that we're done processing.

The passwordsMatch variable in the question is probably a better example than mine.

Single-use variables are useful if they give a meaningful name to some value. (This is of course for the benefit of the human reader.)