PHP Coding styles return; in switch/case

we're trying to implement new coding style guidelines for our team, the php codesniffer is printing an warning on switch case statements when no "break" is found like:

switch ($foo) {
case 1:
return 1;
case 2:
return 2;
default:
return 3;
}

is there any good reason to use :

   switch ($foo) {
case 1:
return 1;
break;
}

?? the break is never reached ?

59605 次浏览
小开

From the PHP manual (http://us3.php.net/manual/en/control-structures.switch.php) :

PHP continues to execute the statements until the end of the switch block, or the first time it sees a break statement. If you don't write a break statement at the end of a case's statement list, PHP will go on executing the statements of the following case. For example:

<?php
switch ($i) {
case 0:
echo "i equals 0";
case 1:
echo "i equals 1";
case 2:
echo "i equals 2";
}
?>

Here, if $i is equal to 0, PHP would execute all of the echo statements! If $i is equal to 1, PHP would execute the last two echo statements. You would get the expected behavior ('i equals 2' would be displayed) only if $i is equal to 2. Thus, it is important not to forget break statements (even though you may want to avoid supplying them on purpose under certain circumstances).

小开

I am not an expert in perfect coding but I think the validator would prefer something like that

switch ($foo) {
case 1:
$ret =  1;
break;
case 2:
$ret = 2;
break;
default:
$ret = 3


}
return $ret

I think using return in case statement to break the flow of the code is not really a best practice. So that's why the validator say there is no break ...

For your question about at category, I don't know ... sorry

小开
最佳答案

It's perfectly valid to leave out the break when you return from a switch.

But it's fairly common practise to add explicit breaks to every case as a defensive programming practise.

switch ($foo) {
case 1:
return 1;
break;


case 2:
return 2;
break;
}

The idea is that should you later change your code in case 1 and remove the return statement, you could forget to add a break.

That would accidentally cause program flow to fall through to case 2.

switch ($foo) {
case 1:
somethingDifferent();


case 2:
return 2;
break;
}

Falling through case statements is slightly unusual and you should add a comment to your code when you do it to show that it's intentional.

switch ($foo) {
case 1:
somethingDifferentAndWeWantToDoCase2AsWell();
// fallthrough


case 2:
return 2;
break;
}

As with many defensive programming practises you've got to balance whether the code bloat - which potentially clutters your code and make it less readable - is worth it or not.

小开

I have much better solution.Please follow below code for above switch statment:

$result = 3; // for default case
switch ($foo) {
case 1:
$result = 1;
break;
case 2:
$result = 2;
break;
default:
// do nothing
}
return $result;

It will not result in any error and code is also fine with concepts.

小开

If your "php codesniffer is printing a warning" try to get another better codesniffer and don't forget to try to use the last PHP stable version. You can, of course, write a breakafter one return, but it doesn't make sense, because it will never be read at all. Your code is OK.

Look at this:

$fun = function(int $argument): string {
switch ($argument) {
case 1:
return "one";
case 2:
return "two";
default:
return "more than two";
}
};
$str = $fun(4); // return "more than two"

In my opinion, this is simpler and better: fewer lines => less code to maintain :-)

小开

To answer your question, no there's no good reason to have something that does nothing. Think about it this way, a comment after the return instead of a break saying "don't forget" will have the same affect - none. And put that way it sounds silly, right?

Unless you need to set a var to use later, I'd suggest the approach you have is perfectly fine. I knew the code's intent within 2 seconds from looking at it. Having a break just creates confusion.

There is no one size fits all really. The correct approach depends on whichever fits the scenario. Set a variable in each case and having a break may be the right way, or perhaps just return makes sense.

 
 

Some observations on other suggestions made in answers:

1) Not having a ABC0 after return means problems could arise if code is later changed

Whenever possible, code should be explicit, as well as readable and clear. We can also code in a way to make future changes easier. But in something as simple as a switch it should be no problem and need no safety net to refactor a case later to add or remove a return or break.

In fact, if you removed a return and "didn't notice there was no break" then that's a poor mistake and could be made in any part of coding. No gotcha checking will save you from that. And one should be very careful coding for future potentials, as that potential may never happen, or something else may happen, and you just end up maintaining obsolete code for years.

In the same vein this was argued to be a safety net for future changes - What if you remove the return and accidentally left in that safety net break when you should have removed it?

Even if this switch statement was a life or death scenario, really serious code, I would be against adding the "pointless" break after the return. Just make sure whoever was working on the code knew what they were doing, and it was code reviewed by enough eyes and tested fully.
If it was that serious, then you'd have additional checks in place better than a proposed safety net to catch sloppy devs.

To argue that break after return adds a safety net, means you're not coding or testing properly. If this is a safety net deemed useful then it's likely there are tons of bugs in the code in potentially more serious places.

The wiki article of "Defensive Programming" was linked to, but it's not relevant here:

Defensive programming is a form of defensive design intended to ensure the continuing function of a piece of software under unforeseen circumstances.

Leaving a safety net break in is not a scenario of unforeseen circumstances, nor defensive programming. It's just bad coding, and you can't litter your code with back up code just in case you don't code correctly when you change something. That's such a bad approach to coding. The argument that "if someone removed return it won't work", well you could also have a typo in the case var, or forget to write the case, or...

The return returns, and you don't code "defensively" to avoid a return failing. That would mean PHP is broken, and you aint gonna fill your code with safety nets to cater for that. That's something you have on a much higher level up.

2) ABC0 after return keeps it explicit

But it's explicitly wrong. The return returns, so the break won't happen. To me that is scratch head time wondering if I've missed the intent - not for long as it's clear what will happen, but there will be a moment where I ponder it to make sure I've not missed something.

While it's not invalid or error to have a return and then break in the same case, it's just entirely pointless as the break does nothing. It's pointless code that needs to be seen, maintained, and figured out as it's not logical.

If explicit is the core goal and having a break after a return urks you because it's pointless, then I'd say it'd be better to set a variable and break, then return the variable after breaking from the switch.
Like @RageZ answer https://stackoverflow.com/a/1437476/2632129

 

3) Set a variable and return after the switch statement is completed

There's nothing wrong with this approach at all, but if there's no reason to store the value in a variable (later use etc) then it's good to return immediately when there's no need to hang around to do anything else.

That shows clear intent - return a value as soon as the case is matched.


提交答案