Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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

share|improve this question
3  
If you do use a temporary variable, write if (!validate) rather than if (validate == false). – Philip Feb 25 '12 at 18:01
12  
I would name the function something like "CredentialsAreValid()" so you know it should be returning a bool but otherwise yes its good practice – Zachary K Feb 25 '12 at 18:23
3  
IsValidCredentials, although grammatically awkward, is a common format for indicating a boolean return value. – zzzzBov Feb 25 '12 at 20:41
    
@Philip what's the difference between if(!validate) and this if(validate) ?? – KyelJmD Feb 26 '12 at 2:47
    
! is the "NOT" operator, it negates any boolean expression. So if (!validate) is the opposite of if (validate). The if statement will be entered if validate is not true. – Philip Feb 26 '12 at 13:53
up vote 26 down vote accepted

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.

share|improve this answer
3  
+1. Readability is top priority here. – tdammers Feb 25 '12 at 20:34
1  
is that type of readable? like the one I did above – KyelJmD Feb 26 '12 at 2:47
    
@KyelJmD: That depends on the culture you're programming in. In the places I've programmed in, the !ValidateCredentials is more readable because it explicitly says what it is doing in the code. It's very clear. If the function you're calling doesn't have a "self-documenting" name then it might be better to go with the variable. As it stands, I'd recommend omitting the variable and stay with the self-documenting code you have. – Joel Etherton Dec 7 '15 at 20:24
    
ValidateCredentials is not readable in the first place - how does the name tell you what the result means? Much better either CredentialsAreValid or CredentialsAreInvalid. – gnasher729 Dec 7 '15 at 20:48

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

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
1  
Good point about method names, but I would never start a method name with "if". Then the code reads "if if". "Can" is ok: if (cat.canOpenCanOfCatFood()). – kevin cline Feb 25 '12 at 22:48
    
Thanks, @Kevin. I meant 'Is' not 'If'. I edited the answer – Christian Horsdal Feb 26 '12 at 20:11

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.

share|improve this answer

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

share|improve this answer
    
I think, I'd rather drop a comment instead of introducing the done_processing variable. After all, the name done_processing serves the function of a comment. And a real comment allows me to say one or two words about why this condition signals we're done with processing. Not that I'm a great commenter, though, I'd probably do neither in real code... – cmaster Dec 7 '15 at 20:32
    
@cmaster: One problem with comments is that they can very easily get out of sync with the code. A variable named done_processing is a more durable way of expressing the intent. – Keith Thompson Dec 7 '15 at 21:52

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.