views:

264

answers:

10

It occurs to me that there are number of different ways to structure conditional logic. As far as I can see, as long as we set errors to end the script (or you can imagine the same examples but with a return in a function), then the following examples are equal:

Example 1

if($condition1) {
    trigger_error("The script is now terminated");
    }

if($condition2) {
    trigger_error("The script is now terminated");
    }

echo "If either condition was true, we won't see this printed";

Example 2

if(!$condition1) {
    if(!$condition2) {
        echo "If either condition was true, we won't see this printed";
        }
    else {
        trigger_error("The script is now terminated");
        }
    }
else {
    trigger_error("The script is now terminated");
    }

Example 3

if($condition1) {
    trigger_error("The script is now terminated");
    }
else {
    if($condition2) {
        trigger_error("The script is now terminated");
        }
    else {
        echo "If either condition was true, we won't see this printed";
        }
    }

Example 4 -- Adapted from Fraser's Answer

function test($condition) { 
    if($condition) {
        trigger_error("The script is now terminated");
        }   
    }

test($condition1);

test($condition2);

echo "If either condition was true, we won't see this printed";

Personally, I lean towards writing code as in Example 1. This is because I feel that by checking for conditions that end the script (or function) in this way, I can clearly define what the script executed and not executed i.e. everything before the condition has been executed and everything after the line has not. This means when I get an error on line 147, I know immediately what has happened helping me to find a bug faster. Furthermore, if I suddenly realise I need to test $condition2 before $condition1, I can make a change by a simple copy paste.

I see a lot of code written like in Example 2 but for me, this seems much more complex to debug. This is because, when the nesting gets too great, an error will get fired off at some distant line at the bottom and be separated from the condition that caused it by a huge chunk of nested code. Additionally, altering the conditional sequence can be a lot messier.

You could hybrid the two styles, such as in Example 3, but this then seems to overcomplicate matters because all of the 'else's are essentially redundant.

Am I missing something? What is the best way to structure my conditional code? Is there a better way than these examples? Are there specific situations under which one style may be superior to another?

Edit: Example 4 looks quite interesting and is not something I had considered. You could also pass in an error message as a second parameter.

Thanks!

P.S. Please keep in mind that I might need to do some arbitrary steps inbetween checking $condition1 and $condition2 so any alternatives must accommodate that. Otherwise, there are trivially better alternatives such as if($condition1 || $condition2).

+2  A: 

#1 is by far the clearest. However, if somehow the thing that previously ended the execution were changed to do something else, then it would break.

It's still probably best to go with #1, but make sure the thing being used to "stop" is clearly named to indicate that it does stop things, so that someone in 10 years maintaining your code doesn't accidentally break things by changing it.

Amber
+1  A: 

I think your method (example 1) is the most efficient and effective in this type of situation. However, there are times when you do not want any conditions to halt execution and you only want to execute condition2 if condition1 is false. In these situations, an else or elseif works well.

JGB146
+1  A: 

I prefer this style, which doesn't break if either of the conditional blocks are changed so they do not exit execution.

if($condition1) {
    trigger_error("The script is now terminated");
}
if($condition2) {
    trigger_error("The script is now terminated");
}

if (!$condition1 && !$condition2) {
  echo "If either condition was true, we won't see this printed"; 
}

Edit: Missed the PS, so updated code to match the full question details.

True but you still can't execute code between checking $condition1 and $condition2 (see my P.S.). You would be forced to separate the second else if into a else { do stuff here if($condition2) { } } which is essentially example 3. Example 3 is more complex but I like your idea that the code might change to make a terminating part, non-terminating. Not sure if it is worth it for the added complexity, however. I guess it depends on how complex the conditional block is?
Rupert
Sorry, I missed the PS. I'd go with Example #1, but protect the last echo with `if (!$condition1 and !$condition2)` for code readability and to protect the intended logic from future revisions.
+7  A: 

I am in the Example 1 camp. As a rule of thumb, the less indentation needed, the better.

// Exit early if there are errors.
if ($n < 0) {
    die "bad n: $n";
}

// Handle trivial cases without fuss.
if ($n == 0) {
    return 0;
}

/* Now the meat of the function. */
$widget->frob($n);
foreach ($widget->blaxes as $blax) {
    checkFrobbingStatus($blax);
}
// ...And another 20 lines of code.

When you use an if/else and put the success and error code in parallel sections you make it appear as if the two code blocks are equal. In reality, the edge cases and error conditions should be de-emphasized. By intentionally handling errors early and then not putting the "important" code in an else clause I feel like that makes it visually clearer where the important code is.

"Here are all the preconditions. And now here's the good stuff."

John Kugelman
Nicely put - you make a good point. How do you think this fits in relative to integrated error trapping (i.e. making the assumption that error trapping and pre-conditions are different)?
AJ
Personally, `return ($n == 0) ? 0 : frobTheWidget($n);` would be a better way.
RobertPitt
+1 RobertPitt for the cstyle syntax - much under-used!
Fraser
Oops. My point was that the "meat" is indeed meaty. Updated answer. :-)
John Kugelman
+1 Very well put.
Luiz Damim
+1  A: 

I suggest using ‘try‘ clause over any part that can have errors and use ‘throw "error description"‘ each time an error occures(like example #1).
That way you can have error reporting code once in your program (in the ‘catch‘ clause) and splitting code into functions won't be a hussle rewriting error handlig.

Dani
What are the benefits of this over using a custom error handler?
Rupert
+1  A: 

I generally agree with Amber insofar as your first option seems the most legible. This is something I have fought with myself - thus far the only reasoning I have stumbled across is as follows:

  • The first form is clearest when reading through a linear script, so ideal for simple scripts
  • The second form is cleanest when you need to ensure tidy / clean-up operations

I mention the second because this is a sticky point. Each script may be part of a larger system, and in fact the script elements you are injecting the "bail out" code into may be called by multiple places. Throw in some OO and you've got a real potential pickle.

The best rule of thumb I can recommend is that if your scripts are simple and linear, or your are doing rapid prototyping, then you want to use the first form and just kill the execution at that point. Anything more complicated or "enterprise-esque" will benefit from (at least) a modular redesign so you can isolate the method and the call stack - and possibly encapsulation of an OO build.

With some of the more powerful debugging and tracing tools which are available these days, it is becoming more a matter of personal style than necessity. One other option you might consider is to put information in comments before (and possibly after) each bail-out zone which make it clear what the alternative is should the criteria be met (or failed).

Edit:

I'd say Fraser's answer is the cleanest for encapsulation. The only thing I would add it that you might benefit from passing an object or hash array into the standard "bail out, I'm dead" method so you can modify the information made available to the function without changing parameter lists all the time (very annoying...).

That said - be careful in production systems where you may need to clean up resources in an intermediate state.

AJ
+3  A: 

Personally I hate nested if-else statements so #1 for me from your examples. The other option I would look at is something like the following.

function test($condition) { 
  if($condition) {
    trigger_error("The script is now terminated");
  }   
}

test($condition1);

//do stuff...

test($condition2);

//passed the tests

EDIT: The more I think about it a functional approach is by far the best way in that it negates having to write the logic that tests the conditions more than once. It also allows greater readability because it is obvious you are 'testing' the condition (as long as you give the function a meaningful name). Also, as pointed out in the question edit it would be trivial to pass other parameters to the function. i.e.

function test($c, $msg) { 
  if($c) {
    trigger_error($msg);
  }   
}

test($condition1, "condition1 error");
test($condition2, "condition2 error");
Fraser
Let's start over again, but without the knives this time.
Will
amen to that :)
Fraser
A: 

The best way to structure conditional logic is to follow the logic itself.

If you have dependencies, say, failing of first condition will make others unnecessary is one thing. Then return, goto, nested conditions and exceptions at your choice.

If you are about to make decision upon a test, say

if (!isset($_GET['id'])) { 
  //listing part: 
} else { 
  // form displaying part: 
}  

it's else, elseif and case realm.

etc.
Determine your program logic first and write it's logic then.

and trigger_error() has nothing to do with conditions. It is debugging feature, not program logic related one.

Col. Shrapnel
Your answer is tautological (the best way to structure logic is logically) and does not even address the question. Also, -1 for suggesting 'goto' as a way to help structure conditional logic!
Fraser
I agree with Fraser - this is not really an answer. Also, the time for <code>GOTO</code> is mostly a remnant of older languages and not a good approach in most modern languages.
AJ
OMG bunch of idiots
Col. Shrapnel
@Col now why is it you keep popping up in the moderation queue so often? Take a day or two to think about it mkay?
Will
A: 

I much prefer #1 as well.

In addition, I really like to assign variables during the conditionals

e.g.

if ( !$userName = $user->login() ) {
    die('could not log in');
}

echo "Welcome, $username";

I usually find that in the first write of code, I end up with a fair few messy nested conditional statements, so it's usually during a second pass that I go back and clean things up, un-nest as many of the conditionals that I can.

In addition to looking much neater, I find it conceptually much easier to understand code where you don't have to mentally keep track of branching logic.

For branching logic that can't be removed that contains lots of procedural code, I usually end up putting it in a function / class method -- ideally so I can see on one screen all the branching logic that is taking place, but modifying either the actions or the logic won't break the other one.

Jhong
Although you do still end up with the problem (potentially) of your application being in an inconsistent state or having resource issues. Also, with production systems you usually can't just dump out like that - error messages and alternative flows are usually a must.
AJ
and also `if ( !$userName = $user->login()){` should be `if( false != ($userName = $user->login()) ){`
RobertPitt
@AJ: Sure. die() just to fill the gap. Robert: No, if I get my login() method to return false on failure, and a non-empty string on successful login, then this will work as expected.
Jhong
A: 

Example 5

if($condition1 || $condition2)
{
    echo "If either condition was true, we won't see this printed";
}else
{
    trigger_error("The script is now terminated");
}

Example 6

function allAreTrue()
{
    foreach(func_get_args() as $check)
    {
       if(!$check)
       {
           return false;
       }
    }
    return true;
}

if(allAreTrue(true,true,$condition1,$condition2,false))
{
   exit("Invalid Arguments");
}

//Continue 
RobertPitt
Both examples ignore - "I might need to do some arbitrary steps inbetween checking $condition1 and $condition2 so any alternatives must accommodate that..." Also, Example 6 "allAreTrue" always returns false...it should be;function allAreTrue(){ foreach(func_get_args() as $check) { if($check) { return false; } } return true;}
Fraser
yea small overlook!
RobertPitt
6 is the same stupid approach as fraser's. you can't sent conditions just like function params. What's the use of these booleans? Real condition can be SQL query result, function call, logical operator, anything. but just boolean? where you gonna get it? another condition somewhere else?
Col. Shrapnel
While 5 is all right and one of the few sensible examples here
Col. Shrapnel
@ Col. Shrapnel - Example 5 does not answer the question. In very simple terms the boolean condition could be the result of any operation. If you don't understand this then you don't understand the most basic of programming concepts.
Fraser
@Col - p.s. you seem to understand well enough when YOU use the EXACT same logic in your answer here: http://stackoverflow.com/questions/2573704/what-are-the-characteristics-of-spaghetti-code/2573731#2573731
Fraser