4
\$\begingroup\$

I'm working on an app that will be used for employee management and I'm using MySQL for the database. You can add employees, view/update/delete them, and on the dashboard I'm listing them in different tables depending on what's needed.

I was wondering if my insert and update code can be improved somehow. Because my update code I think it looks hard to maintain/edit. I was also wondering if there is a more simple way to do it. It's huge and I'm not even at the middle of adding all of the fields I need to edit/update info for.

This is my insert code (for adding new employees to my db):

<?php $server = "localhost"; $user = "root"; $pass = ""; $dbname = "employees"; // Create connection $conn = mysqli_connect($server, $user, $pass, $dbname); // Check connection if (!$conn) { die("Connection failed: " . mysqli_connect_error()); } $fname = mysqli_real_escape_string($conn, $_POST['fname']); $lname = mysqli_real_escape_string($conn, $_POST['lname']); $dob = mysqli_real_escape_string($conn, $_POST['dob']); $embg = mysqli_real_escape_string($conn, $_POST['embg']); $address = mysqli_real_escape_string($conn, $_POST['address']); $city = mysqli_real_escape_string($conn, $_POST['city']); $mobile = mysqli_real_escape_string($conn, $_POST['mobile']); $email = mysqli_real_escape_string($conn, $_POST['email']); $workplace = mysqli_real_escape_string($conn, $_POST['workplace']); $workposition = mysqli_real_escape_string($conn, $_POST['workposition']); $jobstartdate = mysqli_real_escape_string($conn, $_POST['jobstartdate']); $contractfrom = mysqli_real_escape_string($conn, $_POST['contractfrom']); $contractto = mysqli_real_escape_string($conn, $_POST['contractto']); $healthbookfrom = mysqli_real_escape_string($conn, $_POST['healthbookfrom']); $healthbookto = mysqli_real_escape_string($conn, $_POST['healthbookto']); $bankaccount = mysqli_real_escape_string($conn, $_POST['bankaccount']); $bank = mysqli_real_escape_string($conn, $_POST['bank']); $workcode = mysqli_real_escape_string($conn, $_POST['workcode']); $gender = mysqli_real_escape_string($conn, $_POST['gender']); $bloodtype = mysqli_real_escape_string($conn, $_POST['bloodtype']); $notes = mysqli_real_escape_string($conn, $_POST['notes']); $contract_file = basename($_FILES['contractupload']['name']); $contract_path = "files/contracts/$contract_file"; $contract_file = mysqli_real_escape_string($conn, $contract_file); copy($_FILES['contractupload']['tmp_name'], $contract_path); // copy the file to the folder $sql = "INSERT INTO addemployees (fname, lname, dob, embg, address, city, mobile, email, workplace, workposition, jobstartdate, contractfrom, contractto, healthbookfrom, healthbookto, contractupload, bankaccount, bank, workcode, gender, bloodtype, notes) VALUES ('$fname', '$lname', '$dob', '$embg', '$address', '$city', '$mobile', '$email', '$workplace', '$workposition', '$jobstartdate', '$contractfrom', '$contractto', '$healthbookfrom', '$healthbookto', '$contract_file', '$bankaccount', '$bank', '$workcode', '$gender', '$bloodtype', '$notes')"; if (mysqli_query($conn, $sql)) { header("location: employees.php"); // echo "New record created successfully"; } else { echo "Error: " . $sql . "<br>" . mysqli_error($conn); } //Close the connection mysqli_close($conn); ?> 

And this is my update user info code:

<?php // Include config file require_once "config.php"; // Define variables and initialize with empty values $fname = $lname = $dob = $embg = $address = $city = $mobile = $email = $workplace = $workposition = $jobstartdate = $contractfrom = ""; $fname_err = $lname_err = $dob_err = $embg_err = $address_err = $city_err = $mobile_err = $email_err = $workplace_err = $workposition_err = $jobstartdate_err = $contractfrom_err = ""; // Processing form data when form is submitted if(isset($_POST["id"]) && !empty($_POST["id"])){ // Get hidden input value $id = $_POST["id"]; // Validate First Name ($fname) $input_fname = trim($_POST["fname"]); if(empty($input_fname)){ $lname_err = "Please enter your First Name."; } else{ $fname = $input_fname; } // Validate Last Name ($lname) $input_lname = trim($_POST["lname"]); if(empty($input_lname)){ $lname_err = "Please enter your Last Name."; } else{ $lname = $input_lname; } // Validate Date of Birth ($dob) $input_dob = trim($_POST["dob"]); if(empty($input_dob)){ $dob_err = "Please enter your Date of Birth."; } else{ $dob = $input_dob; } // Validate EMBG ($embg) $input_embg = trim($_POST["embg"]); if(empty($input_embg)){ $embg_err = "Please enter your EMBG."; } else{ $embg = $input_embg; } // Validate Address ($address) $input_address = trim($_POST["address"]); if(empty($input_address)){ $address_err = "Please enter an address."; } else{ $address = $input_address; } // Validate City ($city) $input_city = trim($_POST["city"]); if(empty($input_city)){ $city_err = "Please enter your City."; } else{ $city = $input_city; } // Validate Mobile Number ($mobile) $input_mobile = trim($_POST["mobile"]); if(empty($input_mobile)){ $mobile_err = "Please enter your Mobile."; } else{ $mobile = $input_mobile; } // Validate E-mail ($email) $input_email = trim($_POST["email"]); if(empty($input_email)){ $email_err = "Please enter your E-mail."; } else{ $email = $input_email; } // Validate WorkPlace ($workplace) $input_workplace = trim($_POST["workplace"]); if(empty($input_workplace)){ $workplace_err = "Please choose your Work Place."; } else{ $workplace = $input_workplace; } // Validate Work Position ($workposition) $input_workposition = trim($_POST["workposition"]); if(empty($input_workposition)){ $workposition_err = "Please choose your Work Position."; } else{ $workposition = $input_workposition; } // Validate Job Start Date ($jobstartdate) $input_jobstartdate = trim($_POST["jobstartdate"]); if(empty($input_jobstartdate)){ $jobstartdate_err = "Please enter your Date of Birth."; } else{ $jobstartdate = $input_jobstartdate; } // Validate Contract From ($contractfrom) $input_contractfrom = trim($_POST["contractfrom"]); if(empty($input_contractfrom)){ $contractfrom_err = "Please enter your Date of Birth."; } else{ $contractfrom = $input_contractfrom; } // Check input errors before inserting in database jobstartdate if(empty($fname_err) && empty($lname_err) && empty($dob_err) && empty($embg_err) && empty($address_err) && empty($city_err) && empty($mobile_err) && empty($email_err) && empty($workplace_err) && empty($workposition_err) && empty($jobstartdate_err) && empty($contractfrom_err)){ // Prepare an update statement $sql = "UPDATE addemployees SET fname=?, lname=?, dob=?, embg=?, address=?, city=?, mobile=?, email=?, workplace=?, workposition=?, jobstartdate=?, contractfrom=? WHERE id=?"; if($stmt = $mysqli->prepare($sql)){ // Bind variables to the prepared statement as parameters $stmt->bind_param("ssssssssssssi", $param_fname, $param_lname, $param_dob, $param_embg, $param_address, $param_city, $param_mobile, $param_email, $param_workplace, $param_workposition, $param_jobstartdate, $param_contractfrom, $param_id); // Set parameters $param_id = $id; $param_fname = $fname; $param_lname = $lname; $param_dob = $dob; $param_embg = $embg; $param_address = $address; $param_city = $city; $param_mobile = $mobile; $param_email = $email; $param_workplace = $workplace; $param_workposition = $workposition; $param_jobstartdate = $jobstartdate; $param_contractfrom = $contractfrom; // Attempt to execute the prepared statement if($stmt->execute()){ // Records updated successfully. Redirect to landing page header("location: employees.php"); exit(); } else{ echo "Something went wrong. Please try again later."; } } // Close statement $stmt->close(); } // Close connection $mysqli->close(); } else{ // Check existence of id parameter before processing further if(isset($_GET["id"]) && !empty(trim($_GET["id"]))){ // Get URL parameter $id = trim($_GET["id"]); // Prepare a select statement $sql = "SELECT * FROM addemployees WHERE id = ?"; if($stmt = $mysqli->prepare($sql)){ // Bind variables to the prepared statement as parameters $stmt->bind_param("i", $param_id); // Set parameters $param_id = $id; // Attempt to execute the prepared statement if($stmt->execute()){ $result = $stmt->get_result(); if($result->num_rows == 1){ /* Fetch result row as an associative array. Since the result set contains only one row, we don't need to use while loop */ $row = $result->fetch_array(MYSQLI_ASSOC); // Retrieve individual field value $fname = $row["fname"]; $lname = $row["lname"]; $dob = $row["dob"]; $embg = $row["embg"]; $address = $row["address"]; $city = $row["city"]; $mobile = $row["mobile"]; $email = $row["email"]; $workplace = $row["workplace"]; $workposition = $row["workposition"]; $jobstartdate = $row["jobstartdate"]; $contractfrom = $row["contractfrom"]; } else{ // URL doesn't contain valid id. Redirect to error page header("location: error.php"); exit(); } } else{ echo "Oops! Something went wrong. Please try again later."; } } // Close statement $stmt->close(); // Close connection $mysqli->close(); } else{ // URL doesn't contain id parameter. Redirect to error page header("location: error.php"); exit(); } } ?> <!DOCTYPE html> <html lang="en"> <head> <meta charset="UTF-8"> <title>Update Record</title> <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.css"> <style type="text/css"> .wrapper{ width: 500px; margin: 0 auto; } </style> </head> <body> <div class="wrapper"> <div class="container-fluid"> <div class="row"> <div class="col-md-12"> <div class="page-header"> <h2>Измени Податоци</h2> </div> <form action="<?php echo htmlspecialchars(basename($_SERVER['REQUEST_URI'])); ?>" method="post"> <div class="form-group <?php echo (!empty($fname_err)) ? 'has-error' : ''; ?>"> <label>Име</label> <input type="text" id="fname" name="fname" class="form-control" value="<?php echo $fname; ?>"> <span class="help-block"><?php echo $fname_err;?></span> </div> <div class="form-group <?php echo (!empty($lname_err)) ? 'has-error' : ''; ?>"> <label>Презиме</label> <input type="text" name="lname" id="lname" class="form-control" value="<?php echo $lname; ?>"> <span class="help-block"><?php echo $lname_err;?></span> </div> <div class="form-group <?php echo (!empty($dob_err)) ? 'has-error' : ''; ?>"> <label>Дата на Раѓање</label> <input type="date" name="dob" id="dob" class="form-control" value="<?php echo $dob; ?>"> <span class="help-block"><?php echo $dob_err;?></span> </div> <div class="form-group <?php echo (!empty($embg_err)) ? 'has-error' : ''; ?>"> <label>ЕМБГ</label> <input type="text" name="embg" id="embg" class="form-control" maxlength="13" value="<?php echo $embg; ?>"> <span class="help-block"><?php echo $embg_err;?></span> </div> <div class="form-group <?php echo (!empty($address_err)) ? 'has-error' : ''; ?>"> <label>Адреса</label> <input type="text" id="address" name="address" class="form-control" value="<?php echo $address; ?>"> <span class="help-block"><?php echo $address_err;?></span> </div> <div class="form-group <?php echo (!empty($city_err)) ? 'has-error' : ''; ?>"> <label>Град</label> <input type="text" name="city" id="city" class="form-control" value="<?php echo $city; ?>"> <span class="help-block"><?php echo $city_err;?></span> </div> <div class="form-group <?php echo (!empty($mobile_err)) ? 'has-error' : ''; ?>"> <label>Мобилен</label> <input type="text" name="mobile" id="mobile" class="form-control" maxlength="9" value="<?php echo $mobile; ?>"> <span class="help-block"><?php echo $mobile_err;?></span> </div> <div class="form-group <?php echo (!empty($email_err)) ? 'has-error' : ''; ?>"> <label>Е-маил</label> <input type="text" name="email" id="email" class="form-control" value="<?php echo $email; ?>"> <span class="help-block"><?php echo $email_err;?></span> </div> <div class="form-group <?php echo (!empty($workplace_err)) ? 'has-error' : ''; ?>"> <label>Работно Место <span style="font-size: 15px; color: rgb(255, 0, 0); margin-right: 15px;">(ПРОВЕРИ)</span></label> <select type="text" name="workplace" id="workplace" class="form-control" value="<?php echo $workplace; ?>"> <option value="Кафич ГТ-1 - Широк Сокак бр. 55">Кафич ГТ-1 - Широк Сокак бр. 55</option> <option value="Кафич ГТ-2 - Широк Сокак бр. 94">Кафич ГТ-2 - Широк Сокак бр. 94</option> <option value="Ланч Бар ГТ - Широк Сокак бр. 55">Ланч Бар ГТ - Широк Сокак бр. 55</option> <option value="Главен Магацин - Боримечка">Главен Магацин - Боримечка</option> </select> <span class="help-block"><?php echo $workplace_err;?></span> </div> <div class="form-group <?php echo (!empty($workposition_err)) ? 'has-error' : ''; ?>"> <label>Работна Позиција <span style="font-size: 15px; color: rgb(255, 0, 0); margin-right: 15px;">(ПРОВЕРИ)</span></label> <select type="text" name="workposition" id="workposition" class="form-control" value="<?php echo $workposition; ?>"> <option value="Келнер">Келнер</option> <option value="Шанкер">Шанкер</option> <option value="Колачи">Колачи</option> <option value="Сладолед">Сладолед</option> <option value="Производство Сладолед">Производство Сладолед</option> <option value="Производство Торти">Производство Торти</option> <option value="Кувар">Кувар</option> <option value="Помошник Кувар">Помошник Кувар</option> <option value="Салатер">Салатер</option> <option value="Пицер">Пицер</option> <option value="Менаџер">Менаџер</option> <option value="Книговодител">Книговодител</option> <option value="Хигиеничар">Хигиеничар</option> <option value="Стражар">Стражар</option> <option value="Магационер">Магационер</option> <option value="Шофер">Шофер</option> <option value="Дистрибутер">Дистрибутер</option> </select> <span class="help-block"><?php echo $workposition_err;?></span> </div> <div class="form-group <?php echo (!empty($jobstartdate_err)) ? 'has-error' : ''; ?>"> <label>Дата на Почнување на Работа <span style="font-size: 15px; color: rgb(255, 0, 0); margin-right: 15px;">(Месец/Ден/Година)</span></label> <input type="date" name="jobstartdate" id="jobstartdate" class="form-control" value="<?php echo $jobstartdate; ?>"> <span class="help-block"><?php echo $jobstartdate_err;?></span> </div> <div class="form-group <?php echo (!empty($contractfrom_err)) ? 'has-error' : ''; ?>"> <label>Договор за работа од <span style="font-size: 15px; color: rgb(255, 0, 0); margin-right: 15px;">(Месец/Ден/Година)</span></label> <input type="date" name="contractfrom" id="contractfrom" class="form-control" value="<?php echo $contractfrom; ?>"> <span class="help-block"><?php echo $contractfrom_err;?></span> </div> <input type="hidden" name="id" value="<?php echo $id; ?>"/> <input type="submit" class="btn btn-primary" value="Submit"> <a href="employees.php" class="btn btn-default">Cancel</a> </form> </div> </div> </div> </div> </body> </html> 
\$\endgroup\$
4
  • \$\begingroup\$The answer below doesn't answer your main question, how to keep the code size at bay. To answer this one you should rewrite it from 1990-x style to 2010-x style namely using a framework. Laravel would be a good choice for you.\$\endgroup\$CommentedDec 24, 2018 at 7:05
  • \$\begingroup\$"I was wondering if my insert and update code can be improved somehow." Both answers attempt to do this very thing. If you have advice on how to improve the code, then you should post your advice as an answer, not a comment. Your comment does not seek additional clarity/details so it is not suitable as a comment. Rather than put down others, put your expertise to good use and post constructive content in the places designed for it.\$\endgroup\$CommentedDec 27, 2018 at 10:51
  • \$\begingroup\$Laravel is a good framework, but we can't recommend using Laravel as a remedy for everything. In fact using a framework for such a small thing doesn't guarantee the project will have smaller code base. Half of this code is HTML which one way or another is still necessary. The other half are validation messages, which of course could be designed better but are also needed.\$\endgroup\$
    – Dharman
    CommentedDec 27, 2018 at 14:55
  • \$\begingroup\$@Dharman "we can't recommend using Laravel as a remedy for everything" is a correct statement by itself, but nowhere did I offer if for this purpose. I proposed to use Laravel to solve a certain problem for which it will be excellent, preventing the code from bloating when new fields are added. Both HTML and validation parts will shrunk catastrophically. You may want to try it to see for yourself.\$\endgroup\$CommentedDec 28, 2018 at 9:10

3 Answers 3

4
\$\begingroup\$

In your INSERT script:

  • Use your config.php file everywhere instead of hardcoding your connection credentials.
  • You should be checking that the POST elements actually exist before trying to access their values as a matter of best practice. I'd probably use a null coalescing operator or perhaps one giant isset() conditional (isset() can handle multiple arguments).
  • If this was my script, I'd probably incorporate stronger validation checks on each incoming value so that the database is kept clean and meaningful. Rather than tell the user when something is missing, ratchet up the value requirements and inform the user that an expected value didn't have the expected format and describe in detail what is expected (dob format, email, bloodtype, gender etc).
  • I'll recommend object-oriented mysqli syntax because it is more concise and in my opinion easier to read and maintain.
  • Using a prepared statement will avoid all of that bloat with value escaping.
  • You must never provide the actual mysql error when your application is public.

In your UPDATE script:

  • I don't know that I like the chained declaration of default empty strings for so many values -- it has a negative impact on readability and maintainability for a slight (unnecessary) benefit in script length.
  • You should be checking that the POST elements actually exist before trying to access/trim() their values as a matter of best practice. I'd probably write the trim() call in the else portion of the condition block.
  • isset($_POST["id"]) && !empty($_POST["id"]) is a redundant check, just remove the isset() condition because !empty() will accomplish the same thing.
  • Rather than using lots of similar yet separate _err variables, just create an $error array and if there are any invalid values passed, just push them into the array. When deciding to proceed with the update query, just check the size of the error array. If !sizeof($errors), then perform the update, else display all of the invalid values.
  • Rather than trimming $id, just cast it as an integer with (int).
  • Jamming a value attribute like value="<?php echo $workposition; ?>" is not going to work in your <select> fields.

As a matter of personal preference, I tend to write all my negative/failure/error outcomes before my successful outcomes in my condition blocks. By writing the SELECT statement last, you can move directly into your html form portion which should make things easier to associate and debug.

Try to avoid single-use variable declarations. If they improve the readability of your code, that can be a sound justification. However, generally your code will be easier to maintain if you have fewer variables in your global scope.

Finally, because you are processing multibyte characters, be sure to do UTF-8 All The Way Through.

\$\endgroup\$
7
  • 1
    \$\begingroup\$First of all thank you very much for having the time to read my post and go through my code (i know it's huge and i'll work on it). Thank you very much for all of your suggestion. I'll go through all of them individually and 'fix' my code. Again thanks a lot man.\$\endgroup\$
    – Blagojce
    CommentedDec 24, 2018 at 0:02
  • \$\begingroup\$There are probably more refinements to make. Be sure to check back for other volunteers' reviews that may come later.\$\endgroup\$CommentedDec 24, 2018 at 0:05
  • \$\begingroup\$Why should they put trim in the else part of the condition? Isn't it better to get rid of spaces before checking if the input is empty? What if the user accidentally entered a space? Without trimming, your application will accept the space as valid input and then trim it producing empty value.\$\endgroup\$
    – Dharman
    CommentedDec 26, 2018 at 23:34
  • \$\begingroup\$Why is it better to check the size with !sizeof($errors) of the array instead of plain if(!$errors)?\$\endgroup\$
    – Dharman
    CommentedDec 26, 2018 at 23:35
  • 1
    \$\begingroup\$@Dharman I am talking about Notices not Errors. Using !isset() or empty() is a good way to process submissions that are incomplete/fooled-with. At no point am I endorsing "letting it go", rather I recommend handling the unexpected occurrence directly/sensibly.\$\endgroup\$CommentedDec 27, 2018 at 8:22
4
\$\begingroup\$

Here are my ideas.
For the insert script I have the same recommendations as @mickmackusa. Stay consistent and use object-oriented syntax for mysqli and prepared statements. You might find it easier to use PDO instead of mysqli, but it is probably too late at this stage unless your project is still small and you are willing to swap.

Update script:

  1. Don't create so many aliases on your variables. There is no reason for it if you are not modifying their value. If 2 variables hold the same value, but have different name your code gets harder to understand.

  2. Don't declare empty strings explicitly. Try out my nifty trick of using trim with null-coalesce operator*: trim($_POST['fname'] ?? ''); It doesn't trigger a notice and also defaults to an empty string if the variable doesn't exist. Personally I don't agree here with @mickmackusa statements that your should call isset before trim, I see no benefit in doing so, and much more prefer defaulting it to a null or empty string.

  3. Use isset or empty. There is no need to call them both. Try: if (!empty($_POST['id'])) {

  4. Your id should be an integer so you should enforce that. A short way of doing so would be $id = (int) ($_POST['id'] ?? null);, but keep in mind that your should do more data validation than this!

  5. Use an associative array for your validation errors. This makes the syntax simpler (see answer by @mickmackusa), and still allows you to separate the messages. An empty array is falsish value so you can just check if(!$errors) to see if the validations passed.
    In your HTML form you can then check if the key exists and display the message. Here again you could use null-coalesce operator*: <?php echo $validation_errors['embg'] ?? '';?> to get rid of the pesky notices, or you could redesign your HTML to display the <span> only if the message exists.

  6. Close your mysqli statement in the same code block it was created or not at all, PHP will do it for you anyway. If the prepare call fails it will return FALSE and your can't call FALSE->close(). Close the statement only if prepare was successful.

  7. Exit with a header. Exit can take an argument in the form of a string and header returns nothing which makes it a perfect pair to put together: exit(header('location: employees.php'));. Saves you one line at least.

  8. No need for an else statement after the exit. Exit will terminate the script so else part will never be reached.

  9. You should close mysqli connection either in the same block of code it was created or not at all. When PHP script terminated it will close the connection for you automatically. If you really need to close it yourself don't put it inside an if statement.

  10. The only statement in the else part is an if statement. Use elseif instead. In your case the statement can be if/elseif/else instead of if{if/else}

  11. Prevent XSS! Never output raw data regardless of where it came from. Use htmlspecialchars($str, ENT_QUOTES | ENT_HTML5, 'UTF-8'); on any data displayed into HTML. I have created a wrapper function to make it simpler to call this.

  12. HMTL select tag doesn't have type or value attributes.

* If you are not yet on >=PHP 7, then instead of null coalesce operator you need to use a longer syntax with an isset or create a shim.

<?php // Include config file require_once 'config.php'; // Processing form data when form is submitted if (!empty($_POST['id'])) { // Get hidden input value $id = (int) ($_POST['id'] ?? null); // define an empty array for validation errors to be displayed in HTML form $validation_errors = []; // Validate First Name ($fname) $fname = trim($_POST['fname'] ?? ''); if (empty($fname)) { $validation_errors['fname'] = 'Please enter your First Name.'; } // Validate Last Name ($lname) $lname = trim($_POST['lname'] ?? ''); if (empty($lname)) { $validation_errors['lname'] = 'Please enter your Last Name.'; } // Validate Date of Birth ($dob) $dob = trim($_POST['dob'] ?? ''); if (empty($dob)) { $validation_errors['dob'] = 'Please enter your Date of Birth.'; } // Validate EMBG ($embg) $embg = trim($_POST['embg'] ?? ''); if (empty($embg)) { $validation_errors['embg'] = 'Please enter your EMBG.'; } // Validate Address ($address) $address = trim($_POST['address'] ?? ''); if (empty($address)) { $validation_errors['address'] = 'Please enter an address.'; } // Validate City ($city) $city = trim($_POST['city'] ?? ''); if (empty($city)) { $validation_errors['city'] = 'Please enter your City.'; } // Validate Mobile Number ($mobile) $mobile = trim($_POST['mobile'] ?? ''); if (empty($mobile)) { $validation_errors['mobile'] = 'Please enter your Mobile.'; } // Validate E-mail ($email) $email = trim($_POST['email'] ?? ''); if (empty($email)) { $validation_errors['email'] = 'Please enter your E-mail.'; } // Validate WorkPlace ($workplace) $workplace = trim($_POST['workplace'] ?? ''); if (empty($workplace)) { $validation_errors['workplace'] = 'Please choose your Work Place.'; } // Validate Work Position ($workposition) $workposition = trim($_POST['workposition'] ?? ''); if (empty($workposition)) { $validation_errors['workposition'] = 'Please choose your Work Position.'; } // Validate Job Start Date ($jobstartdate) $jobstartdate = trim($_POST['jobstartdate'] ?? ''); if (empty($jobstartdate)) { $validation_errors['jobstartdate'] = 'Please enter your Date of Birth.'; } // Validate Contract From ($contractfrom) $contractfrom = trim($_POST['contractfrom'] ?? ''); if (empty($contractfrom)) { $validation_errors['contractfrom'] = 'Please enter your Date of Birth.'; } // Check input errors before inserting in database jobstartdate if (!$validation_errors) { // Prepare an update statement $sql = 'UPDATE addemployees SET fname=?, lname=?, dob=?, embg=?, address=?, city=?, mobile=?, email=?, workplace=?, workposition=?, jobstartdate=?, contractfrom=? WHERE id=?'; if ($stmt = $mysqli->prepare($sql)) { // Bind variables to the prepared statement as parameters $stmt->bind_param( 'ssssssssssssi', $fname, $lname, $dob, $embg, $address, $city, $mobile, $email, $workplace, $workposition, $jobstartdate, $contractfrom, $id ); // Attempt to execute the prepared statement if ($stmt->execute()) { // Records updated successfully. Redirect to landing page exit(header('location: employees.php')); // exit with a header } echo 'Something went wrong. Please try again later.'; // Close statement // $stmt->close(); // it's redundant in this context } } } elseif ($id = (int)$_GET['id']) { // Check existence of id parameter before processing further // Prepare a select statement $sql = 'SELECT * FROM addemployees WHERE id = ?'; if ($stmt = $mysqli->prepare($sql)) { // Bind variables to the prepared statement as parameters $stmt->bind_param('i', $id); // Attempt to execute the prepared statement if ($stmt->execute()) { $result = $stmt->get_result(); if ($result->num_rows) { // Fetch result row as an associative array. Since the result set contains only one row, we don't need to use while loop $row = $result->fetch_array(MYSQLI_ASSOC); // Retrieve individual field value $fname = $row['fname']; $lname = $row['lname']; $dob = $row['dob']; $embg = $row['embg']; $address = $row['address']; $city = $row['city']; $mobile = $row['mobile']; $email = $row['email']; $workplace = $row['workplace']; $workposition = $row['workposition']; $jobstartdate = $row['jobstartdate']; $contractfrom = $row['contractfrom']; } else { // URL doesn't contain valid id. Redirect to error page exit(header('location: error.php')); // exit with a header } } else { echo 'Oops! Something went wrong. Please try again later.'; } // Close statement // $stmt->close(); // it's redundant in this context } } else { // URL doesn't contain id parameter. Redirect to error page exit(header('location: error.php')); // exit with a header } // Close connection // $mysqli->close(); // it's redundant in this context function clean_HTML(string $str): string { return htmlspecialchars($str, ENT_QUOTES | ENT_HTML5, 'UTF-8'); } ?> <!DOCTYPE html> <html lang="en"> <head> <meta charset="UTF-8"> <title>Update Record</title> <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.css"> <style type="text/css"> .wrapper{ width: 500px; margin: 0 auto; } </style> </head> <body> <div class="wrapper"> <div class="container-fluid"> <div class="row"> <div class="col-md-12"> <div class="page-header"> <h2>Измени Податоци</h2> </div> <form action="<?php echo htmlspecialchars(basename($_SERVER['REQUEST_URI'])); ?>" method="post"> <div class="form-group <?php echo isset($validation_errors['fname']) ? 'has-error' : ''; ?>"> <label>Име</label> <input type="text" id="fname" name="fname" class="form-control" value="<?php echo clean_HTML($fname); ?>"> <span class="help-block"><?php echo $validation_errors['fname'] ?? '';?></span> </div> <div class="form-group <?php echo isset($validation_errors['lname']) ? 'has-error' : ''; ?>"> <label>Презиме</label> <input type="text" name="lname" id="lname" class="form-control" value="<?php echo clean_HTML($lname); ?>"> <span class="help-block"><?php echo $validation_errors['lname'] ?? '';?></span> </div> <div class="form-group <?php echo isset($validation_errors['dob']) ? 'has-error' : ''; ?>"> <label>Дата на Раѓање</label> <input type="date" name="dob" id="dob" class="form-control" value="<?php echo clean_HTML($dob); ?>"> <span class="help-block"><?php echo $validation_errors['dob'] ?? '';?></span> </div> <div class="form-group <?php echo isset($validation_errors['embg']) ? 'has-error' : ''; ?>"> <label>ЕМБГ</label> <input type="text" name="embg" id="embg" class="form-control" maxlength="13" value="<?php echo clean_HTML($embg); ?>"> <span class="help-block"><?php echo $validation_errors['embg'] ?? '';?></span> </div> <div class="form-group <?php echo isset($validation_errors['address']) ? 'has-error' : ''; ?>"> <label>Адреса</label> <input type="text" id="address" name="address" class="form-control" value="<?php echo clean_HTML($address); ?>"> <span class="help-block"><?php echo $validation_errors['address'] ?? '';?></span> </div> <div class="form-group <?php echo isset($validation_errors['city']) ? 'has-error' : ''; ?>"> <label>Град</label> <input type="text" name="city" id="city" class="form-control" value="<?php echo clean_HTML($city); ?>"> <span class="help-block"><?php echo $validation_errors['city'] ?? '';?></span> </div> <div class="form-group <?php echo isset($validation_errors['mobile']) ? 'has-error' : ''; ?>"> <label>Мобилен</label> <input type="text" name="mobile" id="mobile" class="form-control" maxlength="9" value="<?php echo clean_HTML($mobile); ?>"> <span class="help-block"><?php echo $validation_errors['mobile'] ?? '';?></span> </div> <div class="form-group <?php echo isset($validation_errors['email']) ? 'has-error' : ''; ?>"> <label>Е-маил</label> <input type="text" name="email" id="email" class="form-control" value="<?php echo clean_HTML($email); ?>"> <span class="help-block"><?php echo $validation_errors['email'] ?? '';?></span> </div> <div class="form-group <?php echo isset($validation_errors['workplace']) ? 'has-error' : ''; ?>"> <label>Работно Место <span style="font-size: 15px; color: rgb(255, 0, 0); margin-right: 15px;">(ПРОВЕРИ)</span></label> <select name="workplace" id="workplace" class="form-control" > <option value="Кафич ГТ-1 - Широк Сокак бр. 55">Кафич ГТ-1 - Широк Сокак бр. 55</option> <option value="Кафич ГТ-2 - Широк Сокак бр. 94">Кафич ГТ-2 - Широк Сокак бр. 94</option> <option value="Ланч Бар ГТ - Широк Сокак бр. 55">Ланч Бар ГТ - Широк Сокак бр. 55</option> <option value="Главен Магацин - Боримечка">Главен Магацин - Боримечка</option> </select> <span class="help-block"><?php echo $validation_errors['workplace'] ?? '';?></span> </div> <div class="form-group <?php echo isset($validation_errors['workposition']) ? 'has-error' : ''; ?>"> <label>Работна Позиција <span style="font-size: 15px; color: rgb(255, 0, 0); margin-right: 15px;">(ПРОВЕРИ)</span></label> <select name="workposition" id="workposition" class="form-control" > <option value="Келнер">Келнер</option> <option value="Шанкер">Шанкер</option> <option value="Колачи">Колачи</option> <option value="Сладолед">Сладолед</option> <option value="Производство Сладолед">Производство Сладолед</option> <option value="Производство Торти">Производство Торти</option> <option value="Кувар">Кувар</option> <option value="Помошник Кувар">Помошник Кувар</option> <option value="Салатер">Салатер</option> <option value="Пицер">Пицер</option> <option value="Менаџер">Менаџер</option> <option value="Книговодител">Книговодител</option> <option value="Хигиеничар">Хигиеничар</option> <option value="Стражар">Стражар</option> <option value="Магационер">Магационер</option> <option value="Шофер">Шофер</option> <option value="Дистрибутер">Дистрибутер</option> </select> <span class="help-block"><?php echo $validation_errors['workposition'] ?? '';?></span> </div> <div class="form-group <?php echo isset($validation_errors['jobstartdate']) ? 'has-error' : ''; ?>"> <label>Дата на Почнување на Работа <span style="font-size: 15px; color: rgb(255, 0, 0); margin-right: 15px;">(Месец/Ден/Година)</span></label> <input type="date" name="jobstartdate" id="jobstartdate" class="form-control" value="<?php echo clean_HTML($jobstartdate); ?>"> <span class="help-block"><?php echo $validation_errors['jobstartdate'] ?? '';?></span> </div> <div class="form-group <?php echo isset($validation_errors['contractfrom']) ? 'has-error' : ''; ?>"> <label>Договор за работа од <span style="font-size: 15px; color: rgb(255, 0, 0); margin-right: 15px;">(Месец/Ден/Година)</span></label> <input type="date" name="contractfrom" id="contractfrom" class="form-control" value="<?php echo clean_HTML($contractfrom); ?>"> <span class="help-block"><?php echo $validation_errors['contractfrom'] ?? '';?></span> </div> <input type="hidden" name="id" value="<?php echo $id; ?>"/> <input type="submit" class="btn btn-primary" value="Submit"> <a href="employees.php" class="btn btn-default">Cancel</a> </form> </div> </div> </div> </div> </body> </html> 
\$\endgroup\$
1
  • \$\begingroup\$Mod Note: I removed quite a bunch of comments here. I highly recommend perusing Code Review Chat for extended discussions around posts. Thanks!\$\endgroup\$
    – Vogel612
    CommentedDec 27, 2018 at 16:18
2
\$\begingroup\$

Let me introduce you to loops.

A loop is a very important element of a program, it can relieve us from most fatiguing jobs. However important it is, at the same time it is very easy to implement. Frankly, every time you see repeated blocks in your code, you can tell for sure that they can be replaced with a loop.

So, first of all we must define such blocks in your code. As we can see, it's data validation and form output. Let's see, if we can define variable parts in your repeated code blocks and put them in arrays, and then use these arrays to populate just a single code block in a loop:

<?php // Include config file require_once "config.php"; // Define all inputs $config = [ 'fname' => 'Име', 'lname' => 'Презиме', 'dob' => 'Дата на Раѓање', 'embg' => 'ЕМБГ', 'address' => 'Адреса', 'city' => 'Град', 'mobile' => 'Мобилен', 'email' => 'Е-маил', 'workplace' => 'Работно Место', 'workposition' => 'Работна Позиција', 'jobstartdate' => 'Дата на Почнување на Работа', 'contractfrom' => 'Договор за работа од', ]; // take all field names $fields = array_keys($config); if ($_POST) { // Define an array for errors $errors = []; // Let's collect all values here $input = []; // And let's validate all fields in one simple loop! foreach ($fields as $field) { $input[$field] = trim($_POST[$field]); if (!$input[$field]) { $errors[$field] = true; } } if (!$errors) { if (!empty($_POST['id'])) { $input['id'] = $_POST['id']; $sql = "UPDATE addemployees SET fname=?, lname=?, dob=?, embg=?, address=?, city=?, mobile=?, email=?, workplace=?, workposition=?, jobstartdate=?, contractfrom=? WHERE id=?"; $stmt = $mysqli->prepare($sql); $stmt->bind_param("ssssssssssssi", ...array_values($input)); $stmt->execute(); } else { $sql = "INSERT INTO addemployees SET fname=?, lname=?, dob=?, embg=?, address=?, city=?, mobile=?, email=?, workplace=?, workposition=?, jobstartdate=?, contractfrom=?"; $stmt = $mysqli->prepare($sql); $stmt->bind_param("ssssssssssss", ...array_values($input)); $stmt->execute(); } header("location: employees.php"); exit(); } } elseif(!empty($_GET["id"])) { $sql = "SELECT * FROM addemployees WHERE id = ?"; $stmt = $mysqli->prepare($sql); $stmt->bind_param("i", $_GET["id"]); $stmt->execute(); $input = $stmt->get_result()->fetch_assoc(); if (!$input) { exit("Record not found"); } } else { // Let's fill all fields with empty strings foreach ($fields as $field) { $input[$field] = ""; } } // a shorthand function for htmlspecialchars() function e($str) { return htmlspecialchars($str, ENT_QUOTES, 'utf-8'); } ?> <!DOCTYPE html> <html lang="en"> <head> <meta charset="UTF-8"> <title>Update Record</title> <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.css"> <style type="text/css"> .wrapper{ width: 500px; margin: 0 auto; } </style> </head> <body> <div class="wrapper"> <div class="container-fluid"> <div class="row"> <div class="col-md-12"> <div class="page-header"> <h2>Измени Податоци</h2> </div> <form action="" method="post"> <?php foreach (['fname', 'lname', 'dob', 'embg', 'address', 'city', 'mobile', 'email'] as $field ): ?> <div class="form-group <?= (!empty($errors[$field])) ? 'has-error' : ''; ?>"> <label><?=$config[$field]?></label> <input type="text" id="<?=$field?>" name="<?=$field?>" class="form-control" value="<?= e($input[$field]) ?>"> <span class="help-block">Please enter your <?=$config[$field]?></span> </div> <?php endforeach ?> <div class="form-group <?= (!empty($errors['workplace'])) ? 'has-error' : ''; ?>"> <label>Работно Место <span style="font-size: 15px; color: rgb(255, 0, 0); margin-right: 15px;">(ПРОВЕРИ)</span></label> <select type="text" name="workplace" id="workplace" class="form-control" value="<?= e($input['workplace']) ?>"> <option value="Кафич ГТ-1 - Широк Сокак бр. 55">Кафич ГТ-1 - Широк Сокак бр. 55</option> <option value="Кафич ГТ-2 - Широк Сокак бр. 94">Кафич ГТ-2 - Широк Сокак бр. 94</option> <option value="Ланч Бар ГТ - Широк Сокак бр. 55">Ланч Бар ГТ - Широк Сокак бр. 55</option> <option value="Главен Магацин - Боримечка">Главен Магацин - Боримечка</option> </select> <span class="help-block">Please enter your Работно Место</span> </div> <div class="form-group <?= (!empty($errors['workposition'])) ? 'has-error' : ''; ?>"> <label>Работна Позиција <span style="font-size: 15px; color: rgb(255, 0, 0); margin-right: 15px;">(ПРОВЕРИ)</span></label> <select type="text" name="workposition" id="workposition" class="form-control" value="<?= e($input['workposition']) ?>"> <option value="Келнер">Келнер</option> <option value="Шанкер">Шанкер</option> <option value="Колачи">Колачи</option> <option value="Сладолед">Сладолед</option> <option value="Производство Сладолед">Производство Сладолед</option> <option value="Производство Торти">Производство Торти</option> <option value="Кувар">Кувар</option> <option value="Помошник Кувар">Помошник Кувар</option> <option value="Салатер">Салатер</option> <option value="Пицер">Пицер</option> <option value="Менаџер">Менаџер</option> <option value="Книговодител">Книговодител</option> <option value="Хигиеничар">Хигиеничар</option> <option value="Стражар">Стражар</option> <option value="Магационер">Магационер</option> <option value="Шофер">Шофер</option> <option value="Дистрибутер">Дистрибутер</option> </select> <span class="help-block">Please enter your Работна Позиција</span> </div> <?php foreach (['jobstartdate','contractfrom'] as $field ): ?> <div class="form-group <?= (!empty($errors[$field])) ? 'has-error' : ''; ?>"> <label><?=$config[$field]?></label> <input type="text" id="<?=$field?>" name="<?=$field?>" class="form-control" value="<?= e($input[$field]) ?>"> <span class="help-block">Please enter your <?=$config[$field]?></span> </div> <?php endforeach ?> <?php if (isset($input['id'])): ?> <input type="hidden" name="id" value="<?php echo $input['id']; ?>"/> <?php endif ?> <input type="submit" class="btn btn-primary" value="Submit"> <a href="employees.php" class="btn btn-default">Cancel</a> </form> </div> </div> </div> </div> </body> </html> 

As you can see, this code, although covering both insert and update cases, is almost three times shorter than your initial code.

Moreover, adding a regular field would make it bigger by one single line - a new member in $config variable. Which, I believe, does answer your main question, how to make this code easier to maintain.

Please be advised though: this code is essentially 2000s' PHP at best. It is better than your 1990s' code, but it is still considered outdated nowadays. In 2018 you are supposed to use a framework for such a task. And or a reason:

Your code is oversimplified. In reality you will need different validations, different HTML in different form fields and so on. when you will try to implement all this in raw PHP, your code will start to bloat again. But, given such a task of adding / editing a record in the database is so common, that all popular frameworks offer very sleek solutions, including configurable validations, form generation, SQL automation and many, many more other things that will make even a customized code concise and maintainable.

\$\endgroup\$

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.