views:

371

answers:

10

I'm maintaining some code and have found the following pattern a lot:

var isMale = (row["Gender"].ToString() == "M") ? true : false;

instead of this:

var isMale = (row["Gender"].ToString() == "M");

Is there any reason why anyone would do this? Does anyone think the former is more readable or clearer? Is there some sort of old C "gotcha" that this is a hold-over from?

+15  A: 

I guess if you get paid by the character that would be valid. Other than that I can't think of any reason.

Shaun Bowe
You managed to find the one good reason to do this! That being said, if I was being paid per character, I'd be quitting ;)
Reed Copsey
If I was being paid by character I'd be deciding which island to buy for my very soon retirement.
Matt Greer
@Matt - Unless you were getting paid one Old Turkish Lira per character... then you'd never be able to retire.
SwDevMan81
@Reed: If I were paid per character, I'd see if I could get a COBOL job. Then, after writing a few simple programs, I'd get the bathrooms remodeled and the roof redone.
David Thornley
Sure I could, just write a little utility to add about 30 megs worth of comments to each source file.
Matt Greer
Ha! Then the original developer should have used the actual bool datatype instead of var. That's one extra character that's missing. Or wait...skip the keyword and use the Boolean type directly. Or wait...Use the fully qualified System.Boolean! Cha-ching!
Dr. Wily's Apprentice
If he gets paid by the character, he should write documentation instead of using redundant logic as a form of documentation.
@Matt - That would mean you'd get about $30 per file.
Tomer Vromen
+3  A: 

I think it's totally redundant.

In my experience, this often happens when a conditional statement evolves over time and ends up with an explicit true or false rather than a sub-statement.

John Weldon
I think I see what you mean. I'm not sure if this is the case with the code I'm looking at, since it is pretty consistent.
Chris Dwyer
+3  A: 

I guess some people are not comfortable with assigning anything other that true or false to a boolean variable. Don't know why this is, but I have observed it quite a few times.

So, from that aspect it looks like:

set sarcasm on

bool isMale = (row["Gender"].ToString() == "M"); //BAAAAD

but

bool isMale = (row["Gender"].ToString() == "M") ? true : false; //BETTER

and

bool isMale;
if (row["Gender"].ToString() == "M") 
    isMale = true;
else 
    isMale = false;   // BEST!

set sarcasm off

Luckily Resharper makes short work of all these anti-patterns.

SWeko
Sometimes people/teams will even require the curly braces in such a conditional as a coding standard, adding even more lines to this. And you've reminded me that I need to buy my own ReSharper license since switching jobs and missing it terribly...
David
I don't mind typing a few more characters for better readability. Especially if they're going to make it a var. And while I would prefer to put isMale = true on the same line with the "if", I've seen coding standards that want it not only on a separate line, but also in braces, as David advocates.
DOK
IMHO, coding standards should not be enforced with religious zeal, because there are always scenarios where some standard is counter-intuitive, or actually harms readability.Then, again, I maintain that any developer worth his salt should have a firm grasp on what booleans are...
SWeko
I'm surprised so many people agree with this. I can see the intent in the first example much faster than the last. Are you guys all paid by the character? ;)
Marc
I'm not going to downvote this as I don't want the wrath of the n00b army but you ordered things in reverse order(best to worst). The first one is the most clear to someone who understands that operator== is a boolean expression! "[...] assigning anything other that true or false to a boolean variable[...]" **that expression(`x==y`) only evaluates to true/false!!!** The compiler may optimize that out, but the last solution you cited as being "best" is less likely to be optimized [...]
If you write code like this uniformly, then you will be spending extra effort to ensure that you'll be wasting clock cycles everywhere! If you find this code more readable, ever heard of a comment? There's no need to use an if/else for documentation purposes!
To me, the longer it gets, the more error potential you get in there. If you ever needed to change that code (I know this example a bit strange) to say != "M", and flip some stuff around, your implementation is spread over 4 lines, and you have the potential to totally reverse the conditional logic in the if / else body. The one liner should be readily apparent to any working coder, and places the error potential in one statement. Not everything should be a terse one liner, but that's just a waste of perfectly good bytes.
seanb
@stinky472, @seanb: Sorry if the sarcasm was not obvious, made it clearer
SWeko
phew - ok then - thought maybe you were having a really bad day - Thanks for clarifying sarcasm :)
seanb
+1  A: 

Definitely redundant. Could be a leftover practice from another programming language/environment that the original developer was retaining. I could also potentially see a developer viewing the first line as being more readable in that he/she can quickly see that it's setting a boolean when skimming through the code.

David
+1  A: 

The 2nd way sure makes the most sense and I think it's easier to read.

The 2nd way is a bit more clever. I wouldn't put it past me to do something like you are finding if I was churning out code on a Friday afternoon and my brain was already gone for the weekend :-)

ChrisNel52
+16  A: 

A valid reason? No.

It's usually produced by people who don't really understand that a condition is also in itself an expression, producing a boolean result. In particular, people schooled on a language where this isn't the case, such as many variants of BASIC.

bobince
Knowing what I know (which I agree, is a strange phrase) about the developer, this sounds like the likely reason.
Chris Dwyer
+9  A: 

If you can't trust: (row["Gender"].ToString() == "M") to product true or false correctly, then you probably can't really trust: (row["Gender"].ToString() == "M") ? true : false; either. To be sure, you undoubtedly need to repeat it at least a few more times:

(row["Gender"].ToString() == "M") ? true : false ? true : false ? true : false;

Then again, maybe you can't trust a combination of == and ?: by themselves either -- to be really sure, you probably need at least a bit more redundancy:

if ((row["Gender"].ToString() == "M") ? true : false ? true : false ? true : false == true)
    isMale = true == true;
else
    isMale = false != false;

Hmm...maybe I stayed up too late last night... :-)

Jerry Coffin
for some reason the 'false != false' made me laugh
Neil N
@Neil: I'll count that in the plus column then... :-)
Jerry Coffin
+1. Great and humorous way to point out the logical fallacies with recursive examples to demonstrate the redundancy(redundancy[redundancy]).
+2  A: 

The

if (somebool) return true;
else return false;

"pattern" gets laughed at pretty much everywhere I've ever worked. No reason to do it in my opinion.

kekekela
+2  A: 

Applying the ternary operator ?: for an expression that can only evaluate to true/false (x==y) is just downright redundant (it's just downright redundant [it's redundant]).

The above sentence might be easier to read for someone who doesn't understand English very well as they'll know what to look up first in the dictionary and, if spoken, due to the repetition. Yet for native English speakers, the sentence is awkward and, well, redundant.

In your example, either the operator is being used for documentation purposes, or, without meaning to offend, I suspect that there's a poor understanding of how operators and expressions work.

Whether it's a lack of understanding or some bizarre attempt at documentation, we can do this without redundant operators like this:

var isMale = (row["Gender"].ToString() == "M"); // bool

or...

var isMale = (row["Gender"].ToString() == "M"); // true/false

... or better yet, explicitly specify the appropriate type for 'isMale' instead of relying on implicit typing:

bool isMale = (row["Gender"].ToString() == "M");

I've also seen people pessimize their code this way (or using if/else):

bool something = some_int ? true: false;

There's no need to do this and, while the compiler may optimize this, it is inherently less efficient to rely on branching mechanisms over something as simple as this:

bool something = some_int != 0;

... which has the same effect but without the roundabout process of using conditional branching.

This kind of code is just embarrassing. It's like seeing:

switch (x)
{
    case 1: 
        y = 1;
        break;
    case 2:
        y = 2;
        break;
    case 3:
        y = 3;
        break;
    // etc. for all possible values of x
}

The above would most certainly be considered uber-n00b code by most, yet it is not any less redundant logically than x == y ? true: false or x ? true: false (as opposed to x != 0).

A: 

It depends how tired you were when writing the code :D

Sytone