comment module: minor issue unreachable code
Hi there, In the comment module there is some code that will never be reached: function comment_nodeapi(&$node, $op, $arg = 0) { switch ($op) { case 'load': return db_fetch_array(db_query("SELECT...... break; The return will make it impossible to reach the break. Regards, Hans
On 8/21/07, Hans Wolters <hans@lonki.xs4all.nl> wrote:
Hi there,
In the comment module there is some code that will never be reached:
Hi Hans, the proper place to mention this would be in Drupal's issue queue. You can submit a bug report here: http://drupal.org/node/add/project_issue/drupal/bug andrew
Hi Andrew, On 21-aug-2007, at 21:44, andrew morton wrote:
On 8/21/07, Hans Wolters <hans@lonki.xs4all.nl> wrote:
Hi there,
In the comment module there is some code that will never be reached:
Hi Hans, the proper place to mention this would be in Drupal's issue queue. You can submit a bug report here: http://drupal.org/node/add/project_issue/drupal/bug
Thanks, but currently this page seem to have some problems itself: No content types available. Or is this only for user nobody? Regards, Hans
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hans Wolters schrieb:
Hi Andrew,
On 21-aug-2007, at 21:44, andrew morton wrote:
On 8/21/07, Hans Wolters <hans@lonki.xs4all.nl> wrote:
Hi there,
In the comment module there is some code that will never be reached:
Hi Hans, the proper place to mention this would be in Drupal's issue queue. You can submit a bug report here: http://drupal.org/node/add/project_issue/drupal/bug
Thanks, but currently this page seem to have some problems itself:
No content types available.
Or is this only for user nobody?
Yes. Also, putting a break even if it can't be reached is IMO good coding style. Cheers, Gerhard -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFGy0Rjfg6TFvELooQRAoP7AJ9qd6ZLpupVuYmzmE+14EkMjcToSgCgtXs6 lu1yLkPbrDNieCYKynoiSAg= =M/MK -----END PGP SIGNATURE-----
Hi, On 21-aug-2007, at 22:00, Gerhard Killesreiter wrote:
In the comment module there is some code that will never be reached:
Hi Hans, the proper place to mention this would be in Drupal's issue queue. You can submit a bug report here: http://drupal.org/node/add/project_issue/drupal/bug
No content types available.
Or is this only for user nobody?
Yes.
Also, putting a break even if it can't be reached is IMO good coding style.
I.m.h.o it would then need a rewrite to make sure the switch would return it after it is finished. switch($foo) { case ('bar'): $result = 1; break; case ('otherbar'); $result = 2; break; default: $result = 3; } return $result; // or whatever needs to be done with it. But then again, it's only my opinion. Regards, Hans
I.m.h.o it would then need a rewrite to make sure the switch would return it after it is finished.
switch($foo) {
case ('bar'): $result = 1; break; case ('otherbar'); $result = 2; break; default: $result = 3; }
return $result; // or whatever needs to be done with it.
But then again, it's only my opinion.
This looks like a common coding practice that one would expect to see in a C application, but it is not necessarily suitable in PHP. The reason is that in C, we declare the return type of a function and if a function has a return type then it *must* return a value. Likewise, functions with a 'void' return type cannot return a value. In PHP, we do not declare return types, therefore it is entirely possible that a function may return a value some times (depending on inputs) but not at other times. Many Drupal functions work this way, particularly those functions with an $op parameter as in your original example. We do a switch..case on the $op and might return a value for certain operations (e.g. 'view' or 'load') but not return a value for others (e.g. 'delete'). This is why the return statement cannot be placed after the switch block, because we don't always have a value to return! You are correct to say that the break statement after the return is, technically, redundant as it will never be executed. However, it does help to maintain visual consistency between different case blocks, and it also ensures that the code cannot be broken by someone removing the return statement and forgetting to add a break. switch statements are notoriously easy to mess up in comparison with if..else statements, due to the lack of braces and the occasional confusion over indentation levels. Ensuring that every case has a break statement is a good way of avoiding silly errors (of course, there are *some* occasions where you don't want a break statement at all, but these are relatively unusual and I can't think of any examples in Drupal's code). Regards, Rob Knight
Regards,
Hans
Op dinsdag 21 augustus 2007, schreef Hans Wolters:
I.m.h.o it would then need a rewrite to make sure the switch would return it after it is finished.
This is the defacto standard in Drupal, the most clean, and certainly the best option, IMNHO. Should I put this in the coding style handbook as preferred method? Bèr -- Drupal, Ruby on Rails and Joomla! development: webschuur.com | Drupal hosting: www.sympal.nl
Quoting Bèr Kessels <ber@webschuur.com>:
Op dinsdag 21 augustus 2007, schreef Hans Wolters:
I.m.h.o it would then need a rewrite to make sure the switch would return it after it is finished.
This is the defacto standard in Drupal, the most clean, and certainly the best option, IMNHO.
Should I put this in the coding style handbook as preferred method?
Please do. Earnie
Quoting Gerhard Killesreiter <gerhard@killesreiter.de>:
Also, putting a break even if it can't be reached is IMO good coding style.
Coding style is personal. It is taught to us as we learn. What may be good for one isn't necessarily good for another. That is why we have a Coding Standard document. But I haven't found this issue there. <code style="Earnie"> switch ($myValue) { case (1): { ... } [break | return $case1Result]; case (2): { ... } [break | return $case2Result]; case (n): { ... } [break | return $casenResult]; default: { ... } [break | return $defaultResult]; } <return $switchResult;> </code> As you can see I don't think break is always a good thing, it should either be return or break and return should not be embodied in the case. Also I would never use break and return in the same switch; it should be one or the other and not both. If break is used and a value needs returned from the switch/case operation then I put that on the line with the closing brace of the switch. Earnie
Op woensdag 22 augustus 2007, schreef Earnie Boyd:
As you can see I don't think break is always a good thing, it should either be return or break and return should not be embodied in the case.
having more then one return in a function is generally a (very) bad practice. It makes the code hard to read, and far more complex then needed. Bèr -- Drupal, Ruby on Rails and Joomla! development: webschuur.com | Drupal hosting: www.sympal.nl
Bèr Kessels wrote:
Op woensdag 22 augustus 2007, schreef Earnie Boyd:
As you can see I don't think break is always a good thing, it should either be return or break and return should not be embodied in the case.
having more then one return in a function is generally a (very) bad practice. It makes the code hard to read, and far more complex then needed.
Bèr
I have followed this coding practice (one return) for a long time (almost 20 years) for the reason Ber mentions -- it makes it easier to maintain and debug. Returns that are embedded 10 levels deep within loops and conditionals are harder to find while debugging. However, I do not support making this a coding standard -- maybe just a suggestion. Since we code in an interpretive language, fewer lines of code has it's benefit. And it is certainly easier to code a return statement than to set a flag and/or return value, issue a break, and make sure that the code breaks properly to the final end. What I think is needed is some suggestions and some code samples. If you make it a coding standard, then I'll probably add it to coder, and I think we'll be flagging lots of innocuous returns. For example, I do prefer: function example() { if (variable_get('example', 0)) { // ... do something and return return 'something'; } // ... falls off the end with an empty return } to function example() { if (!variable_get('example', 0)) { return; } return 'something'; } Partially for the single return, but also because the logic is cleaner. -- Doug Green douggreen@douggreenconsulting.com 904-583-3342 Bringing Ideas to Life with Software Artistry and Invention...
On Wednesday, 22. August 2007, Doug Green wrote:
For example, I do prefer:
function example() { if (variable_get('example', 0)) { // ... do something and return return 'something'; } // ... falls off the end with an empty return }
Regarding the structure, I agree. What I don't like is to mix up return statements with and without return value in the same function. That is, imho, harder to grasp for people trying to figure how they should parse the return value, and I believe that when there is something to return at one place in the function then there should always be a return statement for each code path, and each return statement should include a real return value. Like, function example() { if (variable_get('example', 0)) { // ... do something and return return 'something'; } return NULL; } Does the same thing, but instead of the caller not noticing that there's actually some different possibility to 'something', it's instantly recognizable that the function needs to be checked on isset() as well. Also, always having return values for retrieval functions prevents cases like arg() where the docs say that FALSE is returned if the arg is not present, but actually it returns without return statement and thus returns NULL. Stuff like this would not happen with the more verbose approach.
Quoting Bèr Kessels <ber@webschuur.com>:
Op woensdag 22 augustus 2007, schreef Earnie Boyd:
As you can see I don't think break is always a good thing, it should either be return or break and return should not be embodied in the case.
having more then one return in a function is generally a (very) bad practice. It makes the code hard to read, and far more complex then needed.
As you say "generally" not advisable to do. But I think by your second sentence that you mean that it is more difficult to find all of the exit points in the function if more than one return is used. And even though I tend to use only one in longer coded functions I will break from the practice on occasion for smaller line count functions. And thank the Gods that PHP doesn't contain a goto statement but it might someday[1]. [1] http://www.php.net/~derick/meeting-notes.html#adding-goto Earnie
participants (8)
-
andrew morton -
Bèr Kessels -
Doug Green -
Earnie Boyd -
Gerhard Killesreiter -
Hans Wolters -
Jakob Petsovits -
Rob Knight