views:

3467

answers:

7

How would you do this PHP switch statement?

Also note that these are much smaller versions, the 1 I need to create will have a lot more values added to it.

Version 1:

switch ($p) { 
    case 'home': 
    case '': 
     $current_home = 'current';
    break; 

    case 'users.online': 
    case 'users.location': 
    case 'users.featured': 
    case 'users.new': 
    case 'users.browse': 
    case 'users.search': 
    case 'users.staff': 
     $current_users = 'current';
    break;

    case 'forum': 
     $current_forum = 'current';
    break; 
}

Version 2:

switch ($p) { 
    case 'home': 
     $current_home = 'current';
    break; 

    case 'users.online' || 'users.location' || 'users.featured' || 'users.browse' || 'users.search' || 'users.staff': 
     $current_users = 'current';
    break;

    case 'forum': 
     $current_forum = 'current';
    break; 
}

UPDATE - Test Results

I ran some speed test on 10,000 iterations,

Time1: 0.0199389457703 // If statements
Time2: 0.0389049446106 //switch statements
Time3: 0.106977939606 // Arrays

+2  A: 

Version 1 is certainly easier on the eyes, clearer as to your intentions, and easier to add case-conditions to.

I've never tried the second version. In many languages, this wouldn't even compile because each case labels has to evaluate to a constant-expression.

Robert Cartaino
A: 

I definitely prefer Version 1. Version 2 may require less lines of code, but it will be extremely hard to read once you have a lot of values in there like you're predicting.

(Honestly, I didn't even know Version 2 was legal until now. I've never seen it done that way before.)

Josh Leitzel
It is not legal.
too much php
+2  A: 

If anyone else was ever to maintain your code, they would almost certainly do a double take on version 2 -- that's extremely non-standard.

I would stick with version 1. I'm of the school of though that case statements without a statement block of their own should have an explicit // fall through comment next to them to indicate it is indeed your intent to fall through, thereby removing any ambiguity of whether you were going to handle the cases differently and forgot or something.

Mark Rushakoff
+5  A: 

Put those many values into an array and query the array, as the switch-case seems to hide the underlying semantics of what you're trying to achieve when a string variable is used as the condition, making it harder to read and understand, e.g.:

$current_home = null;
$current_users = null;
$current_forum = null;

$lotsOfStrings = array('users.online', 'users.location', 'users.featured', 'users.new');

if(empty($p)) {
    $current_home = 'current';
}

if(in_array($p,$lotsOfStrings)) {
    $current_users = 'current';
}

if(0 === strcmp('forum',$p)) {
    $current_forum = 'current';
}
karim79
+1 The first time I read the question, this solution came to mind first. As the number of cases increase, all you need to do is add values to the array and rely on the in_array to do it's job.
Randell
in my case to do it this way, I would have many different arrays so I would be doing the in_array against several arrays, do you think there is any performance gain in 1 way vs another?
jasondavis
I haven't tried benchmarking it it's easier to read, especially it you'll use constants. How many different arrays (and sizes) are you expecting to use?
Randell
I don't think there will be any perceptible difference in performance. Even if there was, I would probably still stick with the above because it just makes more sense to me in terms of readability and maintainability.
karim79
@Randell this is used for a menu selector on a hgih traffic site so there will only be about 8 arrays and some of them can be up to about 15 items each, I know it's not BIG but it is extra work being done and there is going to be a lot of traffic and this would be something that is done on every single page load. Thats why I am asking
jasondavis
@jasondavis - That should be nothing to worry about at all. You can always use microtime() to compare the performance, but I still think the difference would be negligible or nothing to worry about.
karim79
just for knowledge, here is a similar test I found, in this case it seems in_array might be slower then I had thought http://stackoverflow.com/questions/324665/which-is-faster-inarray-or-a-bunch-of-expressions-in-php
jasondavis
I'm assuming that in_array is using hash behind the scene so it's still faster than checking the cases one by one. And again, it's more readable. You might also want to check the design of your menu. I'm wondering why there are 8 arrays. In any case,i'll go with karim79's answer.
Randell
Whoa! I didn't expect that. The difference is negligible. You don't want to sacrifice code readability for that.
Randell
If performance is really an issue here (see numerous posts about the danger of optimizing too soon), and the look up tables really big enough to worry about the speed of in_array, then a properly indexed database is the right solution. Not to be considered until you KNOW you need to optimize.
Lucky
A: 

I think version 1 is the way to go. It is a lot easier to read and understand.

Josh Curren
A: 

Version 2 does not work!!

case 'users.online' || 'users.location' || ...

is exactly the same as:

case True:

and that case will be chosen for any value of $p, unless $p is the empty string.

|| Does not have any special meaning inside a case statement, you are not comparing $p to each of those strings, you are just checking to see if it's not False.

too much php
ok, would you think that using a switch or if statement is faster then creating multiple arrays and checking if a value is in an array multiple times?
jasondavis
I have never heard or read anything that makes me think a switch() would be faster. An route is still going to do a whole heap of string comparisons.
too much php
here is something that shows an array may be slower http://stackoverflow.com/questions/324665/which-is-faster-inarray-or-a-bunch-of-expressions-in-php
jasondavis
+3  A: 

For any situation where you have an unknown string and you need to figure out which of a bunch of other strings it matches up to, the only solution which doesn't get slower as you add more items is to use an array, but have all the possible strings as keys. So your switch can be replaced with the following:

// used for $current_home = 'current';
$group1 = array(
        'home'  => True,
        );

// used for $current_users = 'current';
$group2 = array(
        'users.online'      => True,
        'users.location'    => True,
        'users.featured'    => True,
        'users.new'         => True,
        'users.browse'      => True,
        'users.search'      => True,
        'users.staff'       => True,
        );

// used for $current_forum = 'current';
$group3 = array(
        'forum'     => True,
        );

if(isset($group1[$p]))
    $current_home = 'current';
else if(isset($group2[$p]))
    $current_users = 'current';
else if(isset($group3[$p]))
    $current_forum = 'current';
else
    user_error("\$p is invalid", E_USER_ERROR);

This doesn't look as clean as a switch(), but it is the only fast solution which doesn't include writing a small library of functions and classes to keep it tidy. It is still very easy to add items to the arrays.

too much php
looks good, i'll play around with it
jasondavis