12

I always use If statement (In C#) as (1. Alternative);

if (IsSuccessed == true)
{
   //
}

I know that there is no need to write "== true" as (2. Alternative));

if (IsSuccessed)
{
   //
}

But, I use it because it is more readable and cause no performance issue. Of course, this is my choice and I know many software developers prefer first alternative. What is the best usage, and Why?

Zaki
  • 6,997
  • 6
  • 37
  • 53
NetSide
  • 3,849
  • 8
  • 30
  • 41
  • 26
    Personally, I'd opt for renaming `IsSuccessed` to either `Succeeded` or `IsSuccess`, but that's another matter. – Dominic Rodger Mar 09 '10 at 08:48
  • 20
    IMO the first alt is seen in code done by newbies whereas experienced coders go with the second one. thats just my observation. – Zaki Mar 09 '10 at 08:50
  • 4
    After you use the second form for awhile, you will consider it more readable. Also, as stated before, the first form looks amateurish. You want to stick with the masses, and the majority of programmers use form #2. – ryeguy Mar 09 '10 at 21:57
  • 3
    I agree with @Zaki this smacks of newb – DevelopingChris Mar 09 '10 at 21:58

13 Answers13

25

I don't like the first option. Not only is it redundant, but a simple typo will introduce a bug.

Consider this

bool b = false;

if (b = true) {
   Console.WriteLine("true");
}

Obviously the code will output "true" but that was probably not the intention of the programmer.

Fortunately tools like Resharper warns against this, but it compiles with the default settings (*).

Using the bool directly will remove the issue entirely.

(*) To be fair, VS also warns against this and if you turn on Warnings as errors it won't even compile.

Brian Rasmussen
  • 114,645
  • 34
  • 221
  • 317
  • 7
    Good answer. While I agree with the other stylistic based answers, this provides a practical reason for omitting the unnecessary comparison. – Greg Beech Mar 09 '10 at 09:09
  • 4
    Although this will compile, you do get a warning, even *without Resharper* - "Assignment in conditional expression is always constant; did you mean to use == instead of = ?" – Nick Mar 09 '10 at 21:55
  • protect oneself from slippery fingers. – kenny Mar 09 '10 at 22:21
  • 1
    @Nick: True, and if you turn on warnings as errors VS will stop you from doing this, but with option two you don't have to do anything to avoid the potential bug. – Brian Rasmussen Mar 10 '10 at 04:35
  • 2
    @Brian: Your answer implies that you need to use tools like Resharper in order to get the warning; Nick is just pointing out that this is not so. Anyway, I agree with the rest of your answer. – Gorpik Mar 10 '10 at 08:54
  • @Gorpik: Please see my comment above. VS does warn about this, but R# throws it in your face, which is why I highlighted R# over VS. – Brian Rasmussen Mar 10 '10 at 09:01
  • @Nick + @Gorpik I have updated my answer to reflect the info in the comments. Thanks. – Brian Rasmussen Mar 10 '10 at 09:05
  • When down voting please leave a comment. Thanks. – Brian Rasmussen Mar 21 '10 at 10:09
  • could do this to eliminate the potential of that bug: `if (true == b)` because `if (true = b)` will throw an error in any compiler. – JohnB Aug 24 '10 at 16:23
  • @JohnB: But why not just use booleans as booleans. `if (b)` does the same thing, is shorter and without the risk of errors. – Brian Rasmussen Aug 24 '10 at 16:54
19

I'd personally go for the second option. It reads more naturally and shows that a programmer is actually aware of a built-in bool type, which is a first-class citizen.

Anton Gogolev
  • 113,561
  • 39
  • 200
  • 288
  • 1
    what if it turns to "false" instead of "true". The statement changes as second alternative. Isn't it confusing? – NetSide Mar 09 '10 at 09:00
  • 1
    @Netside: you still only need to do a `if (!IsSuccessed)` ... very compact imo. – Peter Lillevold Mar 09 '10 at 09:04
  • 1
    but, I am not convinced about readability. Do you really think it is more readable? – NetSide Mar 09 '10 at 09:10
  • 8
    Then the design is faulty. You should avoid inherently negative bools: use isOkay instead of isFaulty, hasMembers instead of hasNoMembers, isInWorkMode instead of isInFaultMode, etc. Avoid double and deeper negatives like fire: wrap your head around `if(!noInitErrors)`. The one mistake of designing the system around `if(!isInFaultMode)` as the standard check has bitten me in the ass several times already. – SF. Mar 09 '10 at 09:23
8

Totally style dependant. Seriously. Go with whatever you like for your own stuff, whatever is the enfoced style at your work.

TomTom
  • 61,059
  • 10
  • 88
  • 148
7

I claim that someone favouring the first alternative has a sketchy grasp of boolean logic. They might “understand” it intellectually, but they certainly don’t grok it; they haven’t internalized this way of thinking.

After all, does anyone every use the following idiom? “If it’s raining tomorrow is false we may go swimming” – NO, of course not. nobody says something like this, it’s ridiculous. What argument supports the claim that this idiom suddenly becomes clear when applied in a programming (as opposed to natural) language?

Konrad Rudolph
  • 530,221
  • 131
  • 937
  • 1,214
6

I prefer the second alternative. I think it's more readable, but the first alternative has the advantage of staying the same if you need to use Boolean? for some reason.

Dominic Rodger
  • 97,747
  • 36
  • 197
  • 212
Jens
  • 25,229
  • 9
  • 75
  • 117
  • 2
    Nope, I get a compiler error when trying that. "Cannot implicitly convert type 'bool?' to 'bool' ..." – Jens Mar 09 '10 at 09:02
4

If the name of the boolean value makes it perfectly clear what it is, then I'd always opt for version 2. However, sometimes you're stuck with a particularly obtuse variable name that you can't change, at least, can't change right now... Refactoring is all well and good, but I try and avoid refactoring too heavily when making functional changes to the code as well.

For example:

if (!NoDropDownInHeader == true)
{
  // Activates when there *is* a dropdown in the header)
}

I've actually seen this particular example in production code and simplified it down to:

if (NoDropDownInHeader == false)
{
 // Activates when there *is* a dropdown in the header
}

And I personally think that both examples are more readable (although arguably the first example may be on par with this one for difficulty of mental parsing) than:

if (!NoDropDownInHeader)
{
 // Activates when there *is* a dropdown in the header
}

Note: Yes, I know the variable is badly named, but changing it in the multitude of places that it was present was outside the scope of the change I was making due to the number of places if would affect.

Rob
  • 45,296
  • 24
  • 122
  • 150
  • 5
    Agreed - I always like a "positive" variable i.e. drop the "No" in this case. Makes it much easier to read. Double negatives are not a good thing! – Robert Conn Mar 09 '10 at 09:41
3

I used to write "== true" because I thought it was clearer and more explicit, but decided to change. Now it always seems much clearer without, you'll just get used to it.

openshac
  • 4,966
  • 5
  • 46
  • 77
2

I would settle for your second option aswell. There is in my opinion no need to write the

if (IsSuccessed == true) 
{ 
   // 
}

In fact, I totally dislike the using of == true for a boolean, since it has no extra value. AND: You have to type less characters, which obviously is an advantage :p. To be honest i would also rewrite the boolean to bSuccessed, since it's a boolean.

Younes
  • 4,825
  • 4
  • 39
  • 66
  • 2
    I think it'd be more readable if you read it like "if is successed" than "if bsuccessed". Seems more natural to me. – Ondrej Slinták Mar 09 '10 at 08:52
  • 1
    Aha, so a programmer has to write natural language? :) It's smart to use prefixes for a variable. This way you can, without having to search your code, see in one quick look what the type of your variable is. I know that in environments like VS you can just mouseover, but it's usual to use prefixes – Younes Mar 09 '10 at 08:56
  • Arrghhh! don't start the Hungarian debate again! – Benjol Mar 09 '10 at 09:00
  • 4
    Seriously, `bSuccessed` ? Thought we we're done with Hungarian notation years ago. What "value" do the 'b' give that isn't already there? – Peter Lillevold Mar 09 '10 at 09:01
  • 3
    Yes, please, stop with the Hungarian thing. If your code says `if(Success)` and you have to wonder if `Successs` is a `bool` or not, I'd say you have other problems... – Svish Mar 09 '10 at 09:02
  • The smartness of the hungarian notation must be why it's all but extinct :D – Rune FS Mar 09 '10 at 09:05
  • 1
    "but it's usual to use prefixes" ... can you reference some statistics on that? :) – Peter Lillevold Mar 09 '10 at 09:07
2

The two expressions are equivalent in C#, but be aware than in other languages they are not.

For example, in C++, the first option accepts only a boolean value with a value of true. Any other value on IsSuccessed will invalidate the condition.

The second option accepts any value that is "truthy": values like 1, or any non-zero, are also considered valid for the if.

So these conditions will validate:

// Truthy validation (second option)
if(1) {...} //validates
if(2) {...} //validates

While these others will not:

// Equals to true validation (first option)
if(1==true) {...} // does not validate
if(2==true) {...} // does not validate

Again, this doesn't apply to C#, since it only accepts booleans on ifs. But keep in mind that other languages accept more than just booleans there.

kikito
  • 51,734
  • 32
  • 149
  • 189
2

More typing means more chances for bugs. Option 2 all the way...

Smalltown2k
  • 811
  • 8
  • 13
1

i use the first when i started programming but kinda get used to the second option. It also saves time type extra letters.

hallie
  • 2,760
  • 19
  • 27
1

I'll go for the second. It's easier for me at least. In the first alternative I always wonder why the comparison is made. Check the type of the left hand side just to be sure that no developer on acid overloaded the == operator making comparison between his class and bool an option.
The first also leads to bugs the second won't.
if(a) might need to be change to if(a||b) or if(a&&b) in the first version it might end up as if(a == true || b) and if(a == true && b) in the former b is redundant and the latter equals if(a==b)

Kobi
  • 135,331
  • 41
  • 252
  • 292
Rune FS
  • 21,497
  • 7
  • 62
  • 96
1

What i see most is: (what I do)

if (IsSuccessed)
{
   //
}

and as alternative for in C++, for C# it's not needed (see comment):

if (true == IsSuccessed)
{
   //
}

The alternative is to prevent yourself for making the mistake to assign instead of compare. (= vs ==)

RvdK
  • 19,580
  • 4
  • 64
  • 107
  • 4
    You shouldn't apply coding conventions from other languages, that are meant to solve problems that don't exist. In C# you get a warning: **Assignment in conditional expression is always constant; did you mean to use == instead of = ?**. C# is awesome. – Kobi Mar 09 '10 at 09:02
  • hmm nice didn't know that. I saw coworkers use it in C++, I always use the first one myself. – RvdK Mar 09 '10 at 09:49