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

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

Answer 6

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 7

Good practice is to write code that is correct, readable, efficient.

1. Correct: mainly it won't make a difference, sometimes transactional or sequential issues will be more clearly described in a single if statement. I.e. a correct solution that is not a single if will be much more complex. Think functions with side effects.
2. Readability: long names or names that while accurate don't precisely describe you usage, might be usefully replaced with a local variable.
3. Efficient: calling a function more than once within a single expression, is a slight waste of time if the value can not change between calls. While the imrovement is almost certainly inconsequencial, it also aids readability as it means that either the value can not change between calls or that it can change and you want the initial value.

So, the answer is "it depends", with possibly a slight bias towards using a variable. Note that when using a debugger, holding a value in a variable means that you can hang on to it longer, and not keep having to recalculate it (and of course if it does have side effects then, then it changing everytime your debugger refreshes the value wold be a problem).