views:

1391

answers:

14
if ($_GET["asdf"]==1) { /* do something */ }
else { /* do something */ }

////////////////////////////

if ($_GET["asdf"]==1)
    /* do something */
else
    /* do something */

I was told that the first instance was bad by a person I know. I have absolutely no idea whether this is really such bad practice - does it not shorten the amount to type? Or is it because it just makes a mess?

+2  A: 

Generally non-readable code is a bad practice. The single line is more efficient in your typing and saves line numbers but come back to it a year from now or while you're scanning for bugs and it'll make it more difficult.

In my opinion, yes it's bad practice to have single line if statements.

The computer doesn't really care (as far as I can tell) but you should always write your code like it's going to be maintained by a serial killer that knows where you live.

Readable! Easily self-discernable.

jerebear
@jerebear this looks more like a comment than an answer :)
eglasius
Maybe I'm not specific enough, i'll edit.
jerebear
+3  A: 

The problem I've seen is developers not recognizing the {}-less-if when they add code to one of the conditions. Example:

//before
if(something)
    statement;

//after
if(something)
    statement;
    addedstatement;

Obviously, this won't do what they expect.

Corbin March
I'd attribute this to them not understanding the language. I generally don't try to tailor my code to people who don't know the language I'm writing. That said, if in a particular circumstance it becomes less readable, those who are experts may still have difficulty.
firebird84
A: 

Those are two lines long, so not really a single line.

There's nothing wrong with single line ifs when it makes the code easier to read.

For example, something like this:

if (last_item) print ", and " else print ", "

is much better than

if (last_iem)
{
    print ", and "
}
else
{
    print ", "
}
MarkusQ
Both are bad. The best is `echo (last_item) ? ",and" : ", ";`. And try to use echo, by the way.
Bogdan Constantinescu
+17  A: 

The best practice is to write code that others can read and update easily.

Your first form is questionable because it doesn't follow the forms that most PHP developers are used to:

if(condition){
  // code
}else{
  // code
}

// ... or ...

if(condition)
{
  // code
}
else
{
  // code
}

// ... or ...

if(condition){ /* short code */ }else{ /* short code */ }

// ... or ...

condition ? /* short code */ : /* short code */;

Note that this is entirely about standard practice, and doesn't necessarily make sense -- it's only about what other developers are used to seeing.

Your second form, more importantly, isn't so good because it makes it easy for another programmer to make this mistake:

if(condition)
  // code A
else
  // code B
  // code C (added by another programmer)

In this example, the other programmer added code C, but forgot to wrap the whole else block in braces. This will cause problems. You can defend against this by simply wrapping your if and else blocks in braces.

Ron DeVera
Actually, I'm used to having braces on separate lines.
a2h
This is the 2nd best practice - the best is to write in the same style as the orignal author - although I suppose then this is also true :)
Brian
I disagree with point #2. Only a really terrible programmer would do something like add code C without braces.
rlbond
@rlbond: Or a good programmer that makes a mistake. It's been known to happen.
Beska
@Beska Or just a really bad programmer, because a good programmer would pick up on it when they couldn't find the closing brace.
St. John Johnson
+4  A: 

My preference if for consistency... so:

if(...)
{
   statement 1;
   statement 2;
}
else
{
   statement 1;
   statement 2;
}

is no different than:

if(...)
{
   statement 1;
}
else
{
   statement 1;
}

So I always use them because it is consistent and it avoids problems forgetting to add them in later.

However other people will look at my code and think that it is stupid to put in the { and }. They have their reasons, I have mine... I happen to like my reasons more than I like theirs :-)

TofuBeer
Cheers :) I do the same, and for the same reason: not forgetting to add the curly brackets later. I also love { on new line rather than if(contidion){ because it's not always visible.
Bogdan Constantinescu
A: 

This is more coding style than anything else. That said, my personal opinion is that your second example is potentially quite harmful. It's easy enough to accidentally "add a second line to the block" in languages where braces are the only way to create blocks. But in PHP, where an alternate syntax exists, this is even less likely to set off the necessary warning bells:

if ($_GET["asdf"]==1):
    /* do something */
else:
    /* do something */
endif;

Rule of thumb: if you're going to put your "do something" on a separate line, use braces; if you're not going to use braces, put it on the same line!

Ben Blank
+1  A: 

For all but the shortest statements, use the braces and space them accordingly. You want to do this for a few reasons:

  • It's harder to make a mistake about where something goes.

  • It's easier to read.

  • In languages with macro-expansion facilities (e.g. C, C++), failure to include braces will cause confusing logic errors when a macro containing multiple statements is expanded inside of an unbraced if-else.

John Feminella
A: 

I have seen so many third party code with silly issues, that I prefer to use braces all the time. That said I have never felt good on

if(){}
else (){}

I use if(){} on the same line when it is a short instruction and it is alone. If there is an else use the long:

if(checkSomething)
{
   //dosomething
}
else
{
   //doanotherthing
}
eglasius
A: 

I consider the second example to be bad practice. At some point you may want to add a line to the if statement to debug:

if(something)
    echo "something";
    die("testing");
else
    echo "something else";

But instead of putting the die() in the if statement, the lack of braces would execute the die() regardless of whether the condition is true. This could be dangerous; not using braces in this style is bad practice.

Sometimes, however, the first example could be acceptable if the if statement is always going to be one line long. You could even write it without braces:

if($this == true) echo "this is true";

Even if you want to debug this statement, you won't forget to add braces because everything is on one line. The only problem with doing this is code readability. Use the normal style if you want to maintain readability:

if(something) {
    do something;
}
Bryson
A: 

"Both are worse", (c)

dmityugov
A: 

This is something that I actually remember from an employment exam a while back. The code was similar to the following:

if (x == 0)
    x = 2;
else
    print("x is: %d", x); // debugging!
    x = 4;

Most people here can spot the error, but you can really substitute in anything you want as the "bad code" that was inserted. The more subtle error comes when you have an "old version" of something commented out, and somebody un-comments it, and suddenly the second statement is outside the block.

Basically, unless it's a small test application to learn a concept fast, I always bracket (and even in the test apps I usually bracket). It just isn't worth the headache later if I don't, even in 5-line-methods.

Kevin
A: 

Have you ever seen code like this in C or C++?

    /*  Warning:  bogus C code!  */

if (some condition)
        if (another condition)
                do_something(fancy);
else
        this_sucks(badluck);

Either the indentation is wrong, or the program is buggy, because an "else" always applies to the nearest "if", unless you use braces.

(Let's just use python. No brackets, just pure clean whitespaces. :P)

Kalmi
A: 

One major benefit of using multiple lines is ease of debugging. If you have an if else statement all on one line and the debugger tells you that line x blew up, it's more difficult to determine which part of the statement failed. Multiple lines also makes it easier to step through your code using a debugger.

cleverswine
A: 

You should put the "if" and the "do something" on separate lines to make your code friendlier to interactive debuggers.

If you put both the "if" and "do something" on the same line, then you can't set a breakpoint just on the "do something" line.

Corey Trager