Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

If we assume we have this little snippet of code:

string str = "checked";
bool test1;

if (str == "checked")
{
    test1 = true;
}
else
{
    test1 = false;
}

Is it bad practice to change a simple statement like this to the following?:

bool test2 = (str == "checked");

Because they work exactly the same, and work as required, so I can't imagine how it would be. However, as a young, inexperienced programmer I am not aware of whether such a thing is frowned upon or not. Can anyone tell me, if this is NOT ok, why not?

The following test program:

using System;

public class Test
{

    public static void Main()
    {
        string str = "checked";

        bool test1;

        if (str == "checked")
        {
            test1 = true;
        }
        else
        {
            test1 = false;
        }

        bool test2 = (str == "checked");

        bool test3 = (str != "checked");

        Console.WriteLine(test1.ToString());
        Console.WriteLine(test2.ToString());
        Console.WriteLine(test3.ToString());
    }
}

Outputs:

True
True
False

Any insight etc is appreciated.

share|improve this question
    
personally I'd use the shorter non-if way, suspect this should be migrated to code review though? –  jk. May 30 '13 at 15:14
1  
You are on a good path. Next, you can set about eliminating = some_complicated_condition ? true : false;s –  AakashM May 30 '13 at 15:26
1  
possible duplicate of Is the ternary operator evil? –  gnat May 30 '13 at 16:23
2  
There is a another way: boolean test4=isChecked(str) .... boolean isChecked(String str){ return "checked"==str; }. You can use the method name to make this appropriately self-documenting. –  emory May 30 '13 at 18:55
3  
It's a good practice. Also, it's like the FizzBuzz test: any programmer who claims boolean expressions are "difficult to understand" and that "gets it backwards" unless "they are spelled out in a 'long-form' if" should be a NO HIRE. Think about it: if they can't handle this complexity, would you trust them with fixing a problem in production? –  Andres F. May 31 '13 at 0:31
show 1 more comment

7 Answers

up vote 33 down vote accepted

Is it bad practice to change a simple statement like this to the following?:

bool test2 = (str == "checked");

No, it's good practice. To me, the longer code:

if (str == "checked")
{  
    test1 = true;
}
else
{
    test1 = false;
} 

indicates that the programmer doesn't understand Boolean expressions. The shorter form is much clearer. Similarly, don't write:

if (boolean-expression) {
    return true;
} else {
    return false;
}

Just write return boolean-expression;

share|improve this answer
7  
Ohh, and I love those if (var == true) stuff. –  Johannes Kuhn May 30 '13 at 20:09
    
@JohannesKuhn: some programmers seem to believe that every if statement needs a relational operator –  kevin cline May 30 '13 at 21:37
    
+1000 to this answer (if I could). It indeed indicates lack of understanding of boolean expressions. Also, the longer form is never "clearer". No amount of {, } or ; will make a simple boolean expression "clearer". Everyone: please stop equating clarity with needless verbosity. –  Andres F. May 31 '13 at 0:16
    
Awesome! I had a feeling I was right, I just wasn't sure due to my inexperience... –  Felix Weir Jun 3 '13 at 11:24
add comment

I would be very annoyed at someone who used the long form when the statement is that simple.

It hints that the one who wrote it either doesn't understand boolean data types, or doesn't realize == is independent of if.

share|improve this answer
add comment

While the shorter form is more elegant and compact, there's an advantage to the long form: You can easily set a breakpoint at test1 = true, while you can't with the shorter form.

share|improve this answer
add comment

Who will see your code?

If you think that others who might see your code will be confused with one-line if statements, then use the full spelled-out version. Also, if you think that you might accidentally reverse the logic, then spell it out.

Example:
At a previous job, I had coworkers that required the full, spelled out logic. When they tried to use the one-liners, they would get the logic backwards on a regular basis. Where I'm at now, the company is filled with intelligent people who would very rarely (if ever) make that mistake. Here, we nest the ternary if statements with no confusion.

share|improve this answer
2  
People confused by boolean expressions should be educated. If after being taught about them they are still confused, they have no right to be programmers. But no, the needlessly verbose if is never the right answer. (Likewise, you shouldn't stop writing for each in Java simply because some inexperienced programmer is still unfamiliar with them). –  Andres F. May 31 '13 at 0:18
    
PS: I'd be terribly scared of your coworkers who "got the logic backwards". Were there no code reviews? No tests? Did they get fired? Did anyone got fired at your company for incompetence, at all? –  Andres F. May 31 '13 at 0:27
1  
I think this is the right answer. The computer will understand all forms. So you should write it the way your audience prefers. @AndresF in some multidisciplinary workspaces, some people from non-technical backgrounds may look at your code –  emory May 31 '13 at 2:16
2  
This is like saying "let's not use scalpels for surgery, because non-surgeons might not understand them. Let's use kitchen knives instead! They are easier, and everyone has them!" –  Andres F. May 31 '13 at 3:08
1  
LOL. I did mention that it's my previous job, right? I'm glad that you all live sheltered lives away from the throngs of incompetent programmers and inadequate managers. –  Richard May 31 '13 at 13:57
show 1 more comment

Answering under the assumption that this boolean statement will always be this simple, I have no doubt that the simplified one-liner:

bool test2 = (str == "checked");

would be sufficient, and it definitely produces fewer lines of IL. In my experiment with Mono I found the one-line statement produced:

IL_0000: ldstr "test"
IL_0005: stloc.0
IL_0006: ldloc.0
IL_0007: ldstr "test"
IL_000c: call bool [mscorlib]System.String::op_Equality(string, string)
IL_0011: stloc.1
IL_0012: ldloc.1

While the traditional if-else statement produced:

IL_0000: ldstr "test2"
IL_0005: stloc.0
IL_0006: ldloc.0
IL_0007: ldstr "test2"
IL_000c: call bool [mscorlib]System.String::op_Equality(string, string)
IL_0011: brfalse IL_001d

IL_0016: ldc.i4.1
IL_0017: stloc.1
IL_0018: br IL_001f

IL_001d: ldc.i4.0
IL_001e: stloc.1

IL_001f: ldloc.1
IL_0020: ret
IL_0013: ret

It appears that in my case the Mono compiler created several subroutines and manages a stack of calls in order to handle the if-else.

That being said, if your boolean operation becomes any more complex, then for readability's sake, in a professional development environment, I would at least change it to:

if (/*complex boolean operation*/)
    // set state in case of true evaluation
else
    // set state in case of false evaluation

Without further context I can't provide you with a better solution.

share|improve this answer
2  
As I have written under another answer, the difference in IL may not mean anything. The JIT compiler can and most likely will eliminate any difference. I'd also like to object to your last point: If the condition is that complex, it is hard to read regardless how what you do with it. Solving that problem by extracting subexpressions into aptly-named variables improves readability in either case. –  delnan May 30 '13 at 17:13
1  
I tend to agree on both of your points - a complex boolean expression needs to be rethought altogether most likely - Steve McConnell in Code Complete suggests placing such complexity into its own method. At any rate my thoughts were simply that - thoughts, well or not. I hope that they provided some input that was inspiring either in the positive or in the negative. –  nathandelane May 30 '13 at 20:33
add comment

Well, team coding standards trump random opinions, but....

I think the answer depends upon whether you are writing the code or modifying it.

If you are writing the code, the shorter version is almost always going to be easier to read and understand, and that is what is most import at that time.

If you are modifying the code, then the most important thing is that you have a reason to be doing so. Even assuming that you have unit tests covering the method, changing working code into working code isn't very productive. If you are modifying the code anyway, and not just cleaning up statements like this, then including this as an easier to understand alternative is certainly reasonable.

When writing code, first focus on producing working code, then readable code.

share|improve this answer
add comment

At the risk of being downvoted into oblivion, I would say use the long form because of clarity and maintainability. In the example given, the difference is small. But getting in the habit of writing "clever" one-liners will just make it more difficult for anyone else to maintain your code.

share|improve this answer
5  
Aaargh! A boolean expression is not "clever"; it's programming 101! Here, an even clearer longer form for you: if (cond == true) { return true == true; } else if (cond == false) { return true == false; } else { return FILE_NOT_FOUND; }. It's longer, so it must be clearer! –  Andres F. May 31 '13 at 0:22
add comment

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.