views:

229

answers:

10
+1  Q: 

Boolean types

During code review I discovered many places of our C# code that looks like this:

if(IsValid()) {
     return true;
}
else {
     return false;
}

or even "better":

return (IsValid()? true : false);

I always wondered why not just write the code like this:

return IsValid();

This is the way I would write this code. I ain't questioning the skills of the developers, but maybe trying to look into the developer's soul. Why would a developer favor more complex code and not a more simple and intuitive? Or maybe the reason is that it is hard to accept the Boolean type as the first-class citizen?

+13  A: 

I think return IsValid(); is perfectly valid and readable code.

BTW, I would certainly slap anyone who writes (IsValid() ? true : false) in the face. It's unnecessarily complicated.

PS. This is what svn blame is designed for.

Mehrdad Afshari
Seconded. I see the first example a lot in code and don't get it.
Dana
It's a beginner error; I see students doing that a lot during the CS intro course (in scheme). Too bad svn blame will not do the slapping for you :)
Damien Pollet
@Damien Pollet: Yeah, too bad. btw, I mentioned I'll slap the `IsValid() ? true : false`, not `if (IsValid())` and the reason is, if you know how to use `?:` you should be competent enough not to write it like so.
Mehrdad Afshari
+1  A: 

I would also just say "return IsValid();" I think you're 100% correct in doing so

FailBoy
+1  A: 

return IsValid(); is the way to go. less code, more concise - choice of champions

Jason
+2  A: 

The reasons for the first two examples are entirely human:

  • ignorance
  • lack of intellectual involvement with one's code
  • code was refactored, but only half-way

There's no reason not to (I know it's a double negative) go with return IsValid();

Michael Meadows
+3  A: 

If you're absent-minded, it's easy to refactor some code from this:

private bool ConsiderTheOstrich()
{
    /* do ostrich things */

    if(someCondition && unpredictableThing == 5)
        return true;
    else
    {
        // log something
        return false;
    }
}

To this:

private void IsValid() { return (someCondition && unpredictableThing == 5); }

/* ... */

private void ConsiderTheOstrich()
{
    /* do ostrich things */

    if(IsValid())
        return true;
    else
        return false; // ostrichlogger logs it for us now
}

Without noticing the extra opportunity for brevity.

mquander
+1  A: 

I even sometimes see this some of the legacy code I maintain:

bool retValue;
if (IsValid()) 
{
    retValue = true;
}
else 
{
    retValue = false;
}

return retValue;

Are some programmers paid by the character?

Dana
This may just be a result of strict adherence to the coding style of one return statement per method.
JohnFx
@JohnFx -- that may be true too. I also don't like the "only one return statement" philosophy either :P
Dana
I have seen that too!
boredgeek
+5  A: 

Yes, you should do it as you say. These people are doing these overly verbose things because they first learned it that way, perhaps in CS 101, and it never occurs to them to go outside the space of what they know works to see if there is a better, easier way.

This does speak to their competence. Good programmers need to be a little more thoughtful and a lot less hidebound.

See also http://stackoverflow.com/questions/423823#424005, http://stackoverflow.com/questions/423823#423915.

chaos
A: 

Yes, of course return IsValid(); would be most optimal if the only code you have is as above.

I guess what comes into play though is what else is your function doing? The rest of the code might shed more light on why a developer would put an if statement around IsValid().

After all, if it's just returning IsValid() then why isn't the calling code just checking IsValid() directly rather than having this wrapper method.

Robin Day
+2  A: 

First case is easier when you are debugging. When you step through your source, it is easier to find out what the return value is without opening up immediate window or run IsValid(); just to see the return value.

For the first and second case, developer might not be aware that s/he can simply do

return IsValid();

Lastly, a developer might be forced to use first or second syntax due to company policies.

Sung Meister
A: 

Distraction

I know it's occurred in my code before and I can trace that back to when I was interrupted or not paying attention while coding (I solely blame SO!)

Ignorance

Not knowing the better way to do it. We take it for granted that all programmers think logically, but that's not the case. Some coders are going purely on patterns of what they've seen before:

If (integerA == integerB) { //do special stuff }

//Given integer equality; boolean equality ought to look the same...
If (isValid() == true ) { //do special stuff }

Momentum

That's how someone has always done it and therefore that's how they continue to do it.

Gavin Miller