Am I abusing the GOTO function here?

Question

I am building a CRM for my wife and I to use for our business. I have created a page with several goals in mind:

Be able to create a new entry in the database.
Be able to view an existing entry in the database.
Be able to update an existing entry in the database.

I originally had several php files performing this stuff, but have now used the GOTO function to get the code to bounce around to the different parts I need run depending on what is happening all while staying on the same page.

My question is, other than it looking messy, is there a downfall to doing it this way? In the future I will be looking into other and cleaner ways to do it (suggestions are welcome), but this is working for me at the moment and I would like to move on with the project and start building the additional parts I require for the CRM. Think of this as a beta version if you will. If there is some huge drawback to what I have done already, Id rather address it now, but if this is at least mildly reasonable I will push forward.

Here is what I have:

<?php
// Include Connection Credentials
include("../../comm/com.php");

//Connection to Database
$link = mysqli_connect($servername, $username, $password, $dbname);

// Connection Error Check
if ($link->connect_errno) {
    echo "Sorry, there seems to be a connection issue.";
    exit;
}

// Define Empty Temporary Client ID 
$new_client_id ="";

// Define Empty Success Message
$successful ="";

// Define Empty Error Messages
$firstnameErr ="";
$lastnameErr ="";
$addressErr ="";
$cityErr ="";
$stateErr ="" ;
$zipcodeErr ="";
$phoneErr ="";
$emailErr ="";

// CHECK FOR SEARCH PROCESS
if ($_SERVER["REQUEST_METHOD"] == "POST") {
    if (isset($_POST['searched'])) {
        $client_id = $_POST['client_id'];
        $buttontxt = "Update";
        goto SearchReturnProcess;
    }
}

// Retrieve Client ID
if ($_SERVER["REQUEST_METHOD"] == "POST") {
    if (empty($_POST['client_id'])) {
        $buttontxt = "Create Client";
        goto CreatNewClientProcess;
    } else {
        $client_id = $_POST['client_id'];
        $buttontxt = "Update";
        goto UpdateClientProcess;
    }
}

//  CONTINUE FOR NEW CLIENT
CreatNewClientProcess:

// Check For Missing Fields and report
if ($_SERVER["REQUEST_METHOD"] == "POST") {
    if (empty($_POST["firstname"])) {
        $firstnameErr = "First name is a required field - please make entry below";
        goto FinishUpProcess;
    }
    if (empty($_POST["lastname"])) {
        $lastnameErr = "Last name is a required field - please make entry below";
        goto FinishUpProcess;
    }
    if (empty($_POST["email"])) {
        $emailErr = "Email is a required field - please make entry below";
        goto FinishUpProcess;
    }
    if (empty($_POST["phone"])) {
        $phoneErr = "Phone is a required field - please make entry below";
        goto FinishUpProcess;
    }
    if (empty($_POST["address"])) {
        $addressErr = "Address is a required field - please make entry below";
        goto FinishUpProcess;
    }
    if (empty($_POST["city"])) {
        $cityErr = "City is a required field - please make entry below";
        goto FinishUpProcess;
    }
    if (empty($_POST["state"])) {
        $stateErr = "State/Province is a required field - please make entry below";
        goto FinishUpProcess;
    }
    if (empty($_POST["zipcode"])) {
        $zipcodeErr = "Postal code is a required field - please make entry below";
        goto FinishUpProcess;
    }
}

// Prepared Statement For Database Search
if ($stmt = $link->prepare("INSERT INTO client (firstname, lastname, address, city, state, zipcode, phone, email) VALUES (?,?,?,?,?,?,?,?)")){

// Bind Search Variable
$stmt->bind_param('ssssssss', $firstname, $lastname, $address, $city, $state, $zipcode, $phone, $email);

// Define Form Field Input
$firstname = $_POST['firstname'];
$lastname = $_POST['lastname'];
$address = $_POST['address'];
$city = $_POST['city'];
$state = $_POST['state'];
$zipcode = $_POST['zipcode'];
$phone = $_POST['phone'];
$email = $_POST['email'];

// Execute the Statement
$stmt->execute();
}

// Close Statment
$stmt->close();

// Report Successful Entry
$successful = "Client Successfully Created!";

// Define New Client ID
$new_client_id = $link->insert_id;

// FINISH NEW CLIENT PROCESS
goto FinishUpProcess;



// CONTINUE FOR SEARCHED PROCESS
SearchReturnProcess:

// Prepared Statement For Database Search
$stmt = $link->prepare("SELECT firstname, lastname, address, city, state, zipcode, phone, email FROM client WHERE client_id=?");

// Bind Client ID into Statement
$stmt->bind_param('s', $client_id);

// Execute the Statement
$stmt->execute();

// Bind Variables to Prepared Statement
$stmt->bind_result($firstname, $lastname, $address, $city, $state, $zipcode, $phone, $email);

//fetch value
$stmt->fetch();

// Close Statment
$stmt->close();

// FINISH SEARCHED PROCESS
goto FinishUpProcess; 



// CONTINUE FOR UPDATE CLIENT PROCESS
UpdateClientProcess:

// Prepared Statement For Database Search
if ($stmt = $link->prepare("UPDATE client SET firstname=?, lastname=?, address=?, city=?, state=?, zipcode=?, phone=?, email=? WHERE client_id=?")){

// Bind Search Variable
$stmt->bind_param('sssssssss', $firstname, $lastname, $address, $city, $state, $zipcode, $phone, $email, $client_id);

// Define Form Field Input
$firstname = $_POST['firstname'];
$lastname = $_POST['lastname'];
$address = $_POST['address'];
$city = $_POST['city'];
$state = $_POST['state'];
$zipcode = $_POST['zipcode'];
$phone = $_POST['phone'];
$email = $_POST['email'];
$client_id = $_POST['client_id'];

// Execute the Statement
$stmt->execute();
}

// Close Statment
$stmt->close();

// Report Successful Update
$successful = "Client Updated Successfully!";

// FINISH UPDATE PROCESS
goto FinishUpProcess; 



// CONTINUE FOR FINISHING UP PROCESS
FinishUpProcess:

// Disconnect from Database 
mysqli_close($link)
?>


<!DOCTYPE html>
<html>
<head>
<title>Client Information</title>
<link rel="stylesheet" href="styles.css">
</head>

<body>

<div class="container">  

<form id="contact" action="" method="post">

<h4>enter client info below</h4>
<font color="red"><?php echo $successful; ?></font>

<fieldset>
<input name="client_id" value="<?php if (empty($_POST['client_id'])) { echo $new_client_id; } else { echo $_POST['client_id']; } ?>" type="hidden">
</fieldset>

<fieldset>
<font color="red"><?php echo $firstnameErr; ?></font>
<input name="firstname" value="<?php if (isset($_POST['client_id'])) { echo $firstname; } else { echo $_POST['firstname']; } ?>" placeholder="First Name" type="text" tabindex="1" autofocus>
</fieldset>

<fieldset>
<font color="red"><?php echo $lastnameErr; ?></font>
<input name="lastname" value="<?php if (isset($_POST['client_id'])) { echo $lastname; } else { echo $_POST['lastname']; } ?>" placeholder="Last Name" type="text" tabindex="2">
</fieldset>

<fieldset>
<font color="red"><?php echo $emailErr; ?></font>
<input name="email" value="<?php if (isset($_POST['client_id'])) { echo $email; } else { echo $_POST['email']; } ?>" placeholder="Email Address" type="email" tabindex="3">
</fieldset>

<fieldset>
<input name="mailinglist" id="checkbox" type="checkbox" checked>
<label>add to the mailing list</label>
</fieldset>

<fieldset>
<font color="red"><?php echo $phoneErr; ?></font>
<input name="phone" value="<?php if (isset($_POST['client_id'])) { echo $phone; } else { echo $_POST['phone']; } ?>" placeholder="Phone Number" type="tel" tabindex="4">
</fieldset>

<fieldset>
<font color="red"><?php echo $addressErr; ?></font>
<input name="address" value="<?php if (isset($_POST['client_id'])) { echo $address; } else { echo $_POST['address']; } ?>" placeholder="Street Address" type="text" tabindex="5">
</fieldset>

<fieldset>
<font color="red"><?php echo $cityErr; ?></font>
<input name="city" value="<?php if (isset($_POST['client_id'])) { echo $city; } else { echo $_POST['city']; } ?>" placeholder="City" type="text" tabindex="6">
</fieldset>

<fieldset>
<font color="red"><?php echo $stateErr; ?></font>
<input name="state" value="<?php if (isset($_POST['client_id'])) { echo $state; } else { echo $_POST['state']; } ?>" placeholder="State/Province" type="text" tabindex="7">
</fieldset>

<fieldset>
<font color="red"><?php echo $zipcodeErr; ?></font>
<input name="zipcode" value="<?php if (isset($_POST['client_id'])) { echo $zipcode; } else { echo $_POST['zipcode']; } ?>" placeholder="Postal Code" type="text" tabindex="8">
</fieldset>

<fieldset>
<font color="red"><?php echo $countryErr; ?></font>
<input name="country" value="<?php if (isset($_POST['client_id'])) { echo $country; } else { echo $_POST['country']; } ?>" placeholder="Country" type="text" tabindex="9">
</fieldset>


<fieldset>
<input name="vegan" type="checkbox">
<label>Vegan or Vegitarian</label>
</fieldset>

<fieldset>
<input name="smoker" type="checkbox">
<label>Smoker</label>
</fieldset>

<fieldset>
<textarea name="client_notes" placeholder="general notes" tabindex="10"></textarea>
</fieldset>

<fieldset>
<button name="submit" type="submit" data-submit="...Sending"><?php echo $buttontxt; ?></button>
</fieldset>


</form>
</div>

</body>

</html>

Some people might say that if you are using it then you are abusing it :) — John Coleman, Mar 22 '17 at 00:12
There is absolutely no reason to use a GOTO and sertainly no in this situation — RiggsFolly, Mar 22 '17 at 00:14
Old, but still worth a read: http://homepages.cwi.nl/~storm/teaching/reader/Dijkstra68.pdf — John Coleman, Mar 22 '17 at 00:18
It was worth the read. Thank you. It calls out GoTo as being primative and that is certainly how I feel when writing this code. I guess I will look into alternate methods sooner rather than later. This was, however, my first attempt at getting everything in one spot. — inkslinger, Mar 22 '17 at 00:32
@Machavity - hahahaha. Thats about how I feel. But I am new, and this is where I am at. I wouldn't post here for critique if I didn't want to improve. — inkslinger, Mar 22 '17 at 00:49
My braaaaaaain..... issssss..... mellllllllllting... NOOOOOOO....... — AbraCadaver, Mar 22 '17 at 00:50
The long chain of `if`s that each end with `goto FinishUpProcess;` could be replaced by a `if ... elseif` cascade with the internal gotos eliminated. That is the sort of thing you would need to do in order to eliminate the gotos — John Coleman, Mar 22 '17 at 00:51
@AbraCadaver Steady now. Don't [summon the pony](http://stackoverflow.com/a/1732454) — Machavity, Mar 22 '17 at 01:08
@Machavity: It's TOOO late he COOOOOMES! I think the gotos ripped a hole in the the space time continuum. — AbraCadaver, Mar 22 '17 at 03:12

score 2 · Answer 1 · answered Mar 22 '17 at 02:19

I'm not sure I even knew that goto existed in PHP. I've used (and abused) my share of gotos over the years, but not lately. On to the fixes:

1 - Many of your gotos (e.g., SearchReturnProcess) can be replaced with function calls. Instead of making a chunk of code starting with a label (and using goto to get there), make a separate function with the same name function SearchReturnProcess() and put the code there.

2 - For the error processing, use if elseif:

if ($_SERVER["REQUEST_METHOD"] == "POST") {
    if (empty($_POST["firstname"])) {
        $firstnameErr = "First name is a required field - please make entry below";
    } elseif (empty($_POST["lastname"])) {
        $lastnameErr = "Last name is a required field - please make entry below";
    } elseif...

etc. Then you can either make that set of statements end with an else followed by the block of "no error" code, or instead of a bunch of separate errors you can make one generic error variable (e.g., $fieldErr) and after the block have code like if ($fieldErr != '') to handle error display and simply display the errors in one location instead of next to each field.

This all seems very useful. I will take all of this into consideration as I re-edit what I am developing here. Thank you. — inkslinger, Mar 22 '17 at 14:28

score 1 · Accepted Answer · answered Mar 22 '17 at 05:03

Yes.

I won't preach about heresy and blasphemy but show you that most of your GOTOs are simply wrong.

UpdateClientProcess. That's quite strange an idea that you have to validate input for the creation only. It should be always the same for both create and update. So this one is useless and harmful
FinishUpProcess from validation routines. That's awful from the usability point of view. There was an old Chiniese torture when a victim's head was fixed under the dripping tap. Unharmful at first, it drove people crazy in time. So you are doing with your verifications. Why not to check ALL fields and then tell user at once, instead of showing them errors one by one?
FinishUpProcess from saving data. This violates the HTTP protocol rule says that after processing the POST request a server should issue a Location header redirecting a client using GET method. Otherwise if a client would refresh a page, the record will be duplicated.
It looks messy. You said that. It took me a hard time to navigate your code to review it due to its monotonous structure. Code padding was invented on purpose. In Python, for example, you are forced to use padding to distinguish subordinate code blocks.

A proper structure for this code would be like

$errors = [];
if ($_POST) {
    if (empty($_POST["firstname"])) {
        $errors['firstname'] = "First name is a required field - please make entry below";
    }
    // and so on

    if (!$errors) {
        if (empty($_POST['client_id'])) {
            // go for insert
        } else {
            // go for update
        }
        header("Location: .");
        exit;
    }
    $firstname = htmlspecialchars($_POST['firstname']);
    // and so on
}
if (!$errors ) {
    if (!empty($_GET['client_id'])) {
       // search your data from a GET variable
    } else {
        // define empty variables
    }
}
?>
<html goes here>

This makes perfect sense. Ill go back to the drawing board with it. Regarding your third point, Im not sure I understand what that means 100%. That seems not possible without either leaving the page entirely or perhaps having the PHP issue that command at the end of things? Do you mind clarifying or directing toward info about that? Also, regarding the lack of padding, I hate that I am not using padding. I am just being lazy with it, because I don't have a working code editing software at the moment, I have been going directly through the FileManager on my host website. So many space bar taps! — inkslinger, Mar 22 '17 at 14:26
It is possible to issue at the end but it will make your code more complicated, so I wonder why would you need that. And regarding padding, it is not only wrong editor but the structure itself is flat and hard to follow. — Your Common Sense, Mar 23 '17 at 11:29

Am I abusing the GOTO function here?

2 Answers2