Is GOTO a good practice? (in this php particular case?)_问答_开发者

Im having some problems in the way a present an error mensage to my users. I "solved" the problem using two Goto instructions. Please take a look of the code:

<?php require_once("registration/include/membersite_config.php"); ?>
    <!DOCTYPE html>
    <html lang="en">
    <head><?php include_once("parts/head.php"); ?></head>
<body>
<div id="footerfix">
<?php include_once("parts/header.php"); ?>
    <div class="container">
        <div class="hero-unit">
<?php
if (isset($_GET['i'])) {
    unlink("users/thumbs/" . $_SESSION["user_code"] . ".jpg");
    header('Location: profile.php?i=mycv');
}
if (isset($_FILES['avatar']['tmp_name'])) {
    $file_ext = end(explode('.', $_FILES['avatar']['name']));
    if (in_array($file_ext, array('jpg', 'jpeg', 'png', 'gif')) == false) {
        echo("<h2>Error!</h2><p>Your profile photo have to be a picture file.</p>");
        goto nomore;
    }
    $src_size = getimagesize($_FILES['avatar']['tmp_name']);
    if ($src_size['mime'] == 'image/jpeg') {
        $src_img = imagecreatefromjpeg($_FILES['avatar']['tmp_name']);
    } elseif ($src_size['mime'] == 'image/png') {
        $src_img = imagecreatefrompng($_FILES['avatar']['tmp_name']);
    } elseif ($src_size['mime'] == 'image/gif') {
        $src_img = imagecreatefromgif($_FILES['avatar']['tmp_name']);
    } else {
        echo("<h2>Error!</h2><p>Incorrect file format.</p>");
        goto nomore;
    }
    $thumb_w = 150;
    if ($src_size[0] <= $thumb_w) {
        $thumb = $src开发者_如何学运维_img;
    } else {
        $new_size[0] = $thumb_w;
        $new_size[1] = ($src_size[1] / $src_size[0]) * $thumb_w;
        $thumb = imagecreatetruecolor($new_size[0], $new_size[1]);
        imagecopyresampled($thumb, $src_img, 0, 0, 0, 0, $new_size[0], $new_size[1], $src_size[0], $src_size[1]);
    }
    imagejpeg($thumb, "users/thumbs/" . $_SESSION["user_code"] . ".jpg");
    //header('Location: profile.php?i=mycv');
    echo('<h2>Ready!</h2><p>Your profile picture is updated. <a href="profile.php">Go back</a>.</p>');
    nomore:
    echo "</div></div>";
    include_once("parts/footer.php");
    echo "</div></body></html>";
}
?>

I never understood why goto is the worst think that could happened to a code (at least, every one says that), i will like to hear your opinions about this, and if that is really the worst think ever, how still use my code whitout them? Thanks!

Is GOTO a good practice? (in this php particular case?)

The short answers why GOTO is a bad idea: readability suffers. Consider this:

<?php require_once("registration/include/membersite_config.php"); ?>
<!DOCTYPE html>
<html lang="en">
<head><?php include_once("parts/head.php"); ?></head>
  <body><div id="footerfix">
  <?php include_once("parts/header.php"); ?>
    <div class="container">
      <div class="hero-unit">
<?php
if(isset($_GET['i'])){ unlink("users/thumbs/".$_SESSION["user_code"].".jpg"); header('Location: profile.php?i=mycv');}
if(isset($_FILES['avatar']['tmp_name'])){
    $file_ext = end(explode('.',$_FILES['avatar']['name']));
    if(in_array($file_ext,array('jpg','jpeg','png','gif'))==false){
        echo("<h2>Error!</h2><p>Your profile photo have to be a picture file.</p>");
    }
    else {
        $src_size=getimagesize($_FILES['avatar']['tmp_name']);
        if($src_size['mime']=='image/jpeg') {
            $src_img=imagecreatefromjpeg($_FILES['avatar']['tmp_name']);
        } elseif($src_size['mime']=='image/png') {
            $src_img=imagecreatefrompng($_FILES['avatar']['tmp_name']);
        } elseif($src_size['mime']=='image/gif') {
            $src_img=imagecreatefromgif($_FILES['avatar']['tmp_name']);
        } else {
            echo("<h2>Error!</h2><p>Incorrect file format.</p>");
        }
        if(!empty($src_img)) {
            $thumb_w = 150;
            if($src_size[0]<=$thumb_w){
                $thumb=$src_img;
            }else{ 
                $new_size[0] = $thumb_w;
                $new_size[1] = ($src_size[1]/$src_size[0])*$thumb_w;
                $thumb=imagecreatetruecolor($new_size[0],$new_size[1]);
                imagecopyresampled($thumb,$src_img,0,0,0,0,$new_size[0],$new_size[1],$src_size[0],$src_size[1]);
            }
            imagejpeg($thumb,"users/thumbs/".$_SESSION["user_code"].".jpg");
            //header('Location: profile.php?i=mycv');
            echo('<h2>Ready!</h2><p>Your profile picture is updated. <a href="profile.php">Go back</a>.</p>');
        }
    }
}
?>
    </div></div>
    <?php include_once("parts/footer.php"); ?>
</div>
</body>
</html>

Anyway, you should consider separating your template from logic (google "MVC") and use at least functions for complex operations.

Your use of goto here seems reasonable to me, because you're basically using the following pattern:

if (error_condition_1) {
    goto handle_errors;
} else {
    // do stuff
}
if (error_condition_2) {
    goto handle_errors;
} else {
    // do stuff
}
// ...
if (last_error_condition) {
    goto handle_errors;
} else {
    // do stuff
}
// do stuff
handle_errors:
    // handling errors

There are other ways to do this that don't involve goto; for example, you can use a do ... while (false) loop with break statements in the appropriate places. However, this really just obscures the fact that you're using a goto.

goto has pariah status among computer programmers, which these days is largely unfair; there are situations in which it is a perfectly reasonable choice to use goto (not the majority of situations, I hasten to add) but in general, using goto is liable to get you dogpiled whether or not it's a valid design choice.

Your main problem here, as Daniel Brockman says, is that your code is hard to read. It seems to be crammed onto as few lines as you could manage, and is very dense. Look into how others space out their code, on this site and elsewhere, and see whether you agree that code using other conventions is more readable.

In PHP you need to keep in mind that goto is available only from version 5.3, therefore if your code needs to be portable you may want to consider another option.

Your code is a mess. Implementing a GOTO statement would make it a nightmare. Like XKCD show, terrible things will happen. I remember when I started learning C++, the term "Spaghetti code" often referred to goto usage. It will always mess up your execution flow. Not to mention it ruins readability.

In any case, I've restructured the program for you and it should now work.

The script

<?php require_once("registration/include/membersite_config.php"); 
    //redirect user if i is defined
    if(isset($_GET['i'])){ unlink("users/thumbs/".$_SESSION["user_code"].".jpg"); header('Location: profile.php?i=mycv');}
?>
<!DOCTYPE html>
<html lang="en">
<head><?php include_once("parts/head.php"); ?></head>
  <body>
    <div id="footerfix">
        <?php include_once("parts/header.php"); ?>
        <div class="container">
            <div class="hero-unit">
<?php

check_file();

function check_file(){
    //check for file 
    if(isset($_FILES['avatar']['tmp_name'])){
        //get the file extension
        $file_ext = end(explode('.',$_FILES['avatar']['name']));

        //validate extension
        if(in_array($file_ext,array('jpg','jpeg','png','gif')) ==false){
            echo "<h2>Error!</h2><p>Your profile photo have to be a picture file.</p>"; 
            return -1;
        }

        //get the image dimensions
        $src_size = getimagesize($_FILES['avatar']['tmp_name']);

        switch($src_size['mime']){
            case 'image/jpeg':
                $src_img=imagecreatefromjpeg($_FILES['avatar']['tmp_name']); break;
            case 'image/png':
                $src_img=imagecreatefrompng($_FILES['avatar']['tmp_name']); break;
            case 'image/gif':
                $src_img=imagecreatefromgif($_FILES['avatar']['tmp_name']); break;
            default:
                echo("<h2>Error!</h2><p>Incorrect file format.</p>");
                return -1;
        }

        $thumb_w = 150;

        //if image is within allowed dimensions, set thumbnail as image
        if($src_size[0]<=$thumb_w){
            $thumb = $src_img;
        }else{ 
            $new_size[0] = $thumb_w;
            $new_size[1] = ($src_size[1]/$src_size[0])*$thumb_w;
            $thumb= imagecreatetruecolor($new_size[0],$new_size[1]);
            imagecopyresampled($thumb,$src_img,0,0,0,0,$new_size[0],$new_size[1],$src_size[0],$src_size[1]);
        }

        //create the updated image
        imagejpeg($thumb,"users/thumbs/".$_SESSION["user_code"].".jpg");

        echo '<h2>Ready!</h2><p>Your profile picture is updated. <a href="profile.php">Go back</a>.</p>';
    }
}
?>
                </div>
            </div>
            <?php include_once("parts/footer.php"); ?>
        </div>
    </body>
</html>

As you can see, I encapsulated the whole process within a function. This way, you can do a return statement whenever you encounter an error. Making it a function also enables for quick reuse. You could add some parameters to make it more dynamic.

Goto introduces problems by creating spaghetti code! In other words, you will have hard time understanding the flow of the code in a code block! It is always possible to use structured control constructs and eliminate gotos entirely! As famous computer scientist Edsger W. Dijkstra implied "Goto" considered harmful in his 1968 article "A Case against the GO TO Statement" (article retitled by editor Niklaus Wirth as "Go To Statement Considered Harmful"). Take a look at the Wikipedia entry http://en.wikipedia.org/wiki/Control_structures on control structures as a starting point.

The concept is applicable to all programming languages!

GOTO is not a good practice anywhere.

In my opinion a continue or a break would be preferable. If you get tempted to use a goto, think about the flow of your script, it's likely it can be improved.