14

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?

Gordon Gustafson
  • 40,133
  • 25
  • 115
  • 157
Chris
  • 2,959
  • 1
  • 30
  • 46
  • 1
    Yes, explicitly writing "true" and "false" makes it more readable to me, especially when you've made it a vague type (var). It took me a couple more seconds to understand the second one than the first one. YMMV. – DOK Jul 01 '10 at 20:15
  • @DOK Interesting comment. So, if the type was explicitly stated (bool), do you think the first is still more readable? – Chris Jul 01 '10 at 20:23
  • I'd lose the `var` in both cases, but the second case is preferable. By using it you're making the code **much** more unreadable and unmaintainable. – ChrisF Jul 01 '10 at 20:43
  • 3
    I would rather see `bool` than `var`, here. – Brian Jul 01 '10 at 20:43
  • Maybe the coder wanted to support FileNotFound as well (http://thedailywtf.com/Articles/What_Is_Truth_0x3f_.aspx) – Christian Hayter Jul 01 '10 at 20:52
  • @Chris Dwyer I think declaring it as a bool definitely improves readability. Even though I am a heavy user of the conditional ? syntax, and I would probably write my own code in the second format, I don't think that it is the most readable choice. I like its brevity, but for maximum readability, I have to side with @SWeko and Resharper and others, that explicitly stating "true" or "false" is best. – DOK Jul 01 '10 at 20:59
  • 2
    I think the only way code like this would improve readability is if the reader doesn't clearly understand that operator== is a *boolean* operator, but using the ternary operator is completely redundant. If there's difficulty understanding this code or that 'isMale' is going to be a boolean (which shouldn't be a problem given the way it is named), then a comment like: *// bool* would be more appropriate for beginners than a redundant operator, or better yet, make isMale an actual bool. – stinky472 Jul 02 '10 at 05:29
  • 1
    I just don't get why anyone other than a learner would write the first case. Keyboard fetish? Would also prefer it to be a bool rather than a var. If it's learner code, then ok, but I would be very worried if I'd hired even an intern who wrote that. – seanb Jul 02 '10 at 06:03

9 Answers9

19

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
  • 528,062
  • 107
  • 651
  • 834
  • 1
    Knowing what I know (which I agree, is a strange phrase) about the developer, this sounds like the likely reason. – Chris Jul 01 '10 at 20:44
16

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
  • 9,840
  • 11
  • 50
  • 71
  • 4
    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 Jul 01 '10 at 20:02
  • 4
    If I was being paid by character I'd be deciding which island to buy for my very soon retirement. – Matt Greer Jul 01 '10 at 20:17
  • 1
    @Matt - Unless you were getting paid one Old Turkish Lira per character... then you'd never be able to retire. – SwDevMan81 Jul 01 '10 at 20:24
  • @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 Jul 01 '10 at 20:25
  • Sure I could, just write a little utility to add about 30 megs worth of comments to each source file. – Matt Greer Jul 01 '10 at 20:46
  • 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 Jul 01 '10 at 20:59
  • If he gets paid by the character, he should write documentation instead of using redundant logic as a form of documentation. – stinky472 Jul 02 '10 at 06:13
  • @Matt - That would mean you'd get about $30 per file. – Tomer Vromen Jul 02 '10 at 17:46
10

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
  • 476,176
  • 80
  • 629
  • 1,111
4

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
  • 39,849
  • 11
  • 94
  • 127
  • 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 Jul 01 '10 at 20:03
4

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
  • 30,434
  • 10
  • 71
  • 106
  • 2
    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 Jul 01 '10 at 20:07
  • 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 Jul 01 '10 at 20:13
  • 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 Jul 01 '10 at 20:18
  • 3
    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 Jul 01 '10 at 20:30
  • 1
    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 [...] – stinky472 Jul 02 '10 at 06:04
  • 1
    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! – stinky472 Jul 02 '10 at 06:07
  • 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 Jul 02 '10 at 06:20
  • @stinky472, @seanb: Sorry if the sarcasm was not obvious, made it clearer – SWeko Jul 02 '10 at 13:24
  • phew - ok then - thought maybe you were having a really bad day - Thanks for clarifying sarcasm :) – seanb Jul 02 '10 at 22:47
3

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.

heisenberg
  • 9,665
  • 1
  • 30
  • 38
2

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

stinky472
  • 6,737
  • 28
  • 27
1

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
  • 208,112
  • 36
  • 198
  • 279
1

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
  • 14,655
  • 3
  • 30
  • 36