1

As you can see in the script below, I use multiple if statements when checking registration inputs. Is there an easier, less spaghetti?

The script works as is, but i would like it to be neater.

<?php

if (isset($_POST['register'])) {

    $uname = trim($_POST['uName']);
    $email = trim($_POST['email']);
    $pass = trim($_POST['pass']);
    $passCon = trim($_POST['passCon']);

    $uname = strip_tags($uname);
    $email = strip_tags($email);
    $pass = strip_tags($pass);
    $passCon = strip_tags($passCon);

    if (!empty($pass)) {
        if (!empty($email)) {
            if (!empty($uname)) {
                if ($pass == $passCon) {

                    $query = "SELECT username FROM users WHERE username='$uname'";
                    $result = mysqli_query($conn, $query);
                    $checkUsername = mysqli_num_rows($result);

                    if ($checkUsername == 0) {

                        $query = "SELECT email FROM users WHERE email='$email'";
                        $result = mysqli_query($conn, $query);
                        $count = mysqli_num_rows($result);

                        if ($count == 0) {

                            $password = hash('sha256', $pass);
                            $queryInsert = "INSERT INTO users(id, username, email, password, date) VALUES('', '$uname', '$email', '$password', '" . time() . "')";
                            $res = mysqli_query($conn, $queryInsert);

                            if ($res) {
                                $errTyp = "success";
                                $errMsg = "successfully registered, you may login now";
                            }
                        } else {
                            $errTyp = "warning";
                            $errMsg = "Sorry Email already in use";
                        }
                    } else {
                        $errTyp = "warning";
                        $errMsg = "Sorry Username already in use";
                    }
                } else {
                    $errTyp = "warning";
                    $errMsg = "Passwords didn't match";
                }
            } else {
                $errTyp = "warning";
                $errMsg = "You didn't enter a Username";
            }
        } else {
            $errTyp = "warning";
            $errMsg = "You didn't enter an email address";
        }
    } else {
        $errTyp = "warning";
        $errMsg = "You didn't enter a password";
    }
}

Thank you, Jay

melpomene
  • 84,125
  • 8
  • 85
  • 148
JayLewis
  • 27
  • 1
  • 2

2 Answers2

0

The problem you are facing is not at all uncommon. Many programmers have faced this issue. Let me help you along the way restructuring your script.

First of all, let's get rid of the nested if-else statements. They confuse and obfuscate what is really going on.

Version 1:

if (!isset($_POST['register']))
    redirect('register.php'); // Let's assume that redirect() redirects the user to a different web page and exit()s the script.

$uname = $_POST['uName'];
$email = $_POST['email'];
$pass = $_POST['pass'];
$passRepeat = $_POST['passRepeat'];

if (empty($pass)) {
    $errorMessage = "You didn't enter a password";
}

if (empty($email)) {
    $errorMessage = "You didn't enter an email address";
}

if (empty($uname)) {
    $errorMessage = "You didn't enter a Username";
}

if ($pass !== $passRepeat) {
    $errMsg = "Passwords didn't match";
}

$query = "SELECT username FROM users WHERE username='$uname'";
$result = mysqli_query($conn, $query);
$checkUsername = mysqli_num_rows($result);

if ($checkUsername !== 0) {
    $errMsg = 'Sorry Username already in use';
}

$query = "SELECT email FROM users WHERE email='$email'";
$result = mysqli_query($conn, $query);
$count = mysqli_num_rows($result);

if ($count !== 0) {
    $errMsg = 'Sorry Email already in use';
}

$password = hash('sha256', $pass);
$queryInsert = "INSERT INTO users(id, username, email, password, date) VALUES('', '$uname', '$email', '$password', '" . time() . "')";
$res = mysqli_query($conn, $queryInsert);

Note that although this avoids the nested if statements, this is not the same as the original code, because the errors will fall through. Let's fix that. While we are at it, why would we want to return after the first error occurs? Let's return all the errors at once!

Version 2:

$errors = array();

if (empty($pass)) {
    $errors[] = "You didn't enter a password";
}

if (empty($email)) {
    $errors[] = "You didn't enter an email address";
}

if (empty($uname)) {
    $errors[] = "You didn't enter a username";
}

if ($pass !== $passRepeat) {
    $errors[] = "Passwords didn't match";
}

$query = "SELECT username FROM users WHERE username='$uname'";
$result = mysqli_query($conn, $query);
$usernameExists = mysqli_num_rows($result) > 0;

if ($usernameExists) {
    $errors[] = 'Sorry Username already in use';
}

$query = "SELECT email FROM users WHERE email='$email'";
$result = mysqli_query($conn, $query);
$emailExists = mysqli_num_rows($result) > 0;

if ($emailExists) {
    $errors[] = 'Sorry Email already in use';
}

if (count($errors) === 0) {
    $password = hash('sha256', $pass);
    $queryInsert = "INSERT INTO users(id, username, email, password, date) VALUES('', '$uname', '$email', '$password', '" . time() . "')";
    $res = mysqli_query($conn, $queryInsert);

    redirect('register_success.php');
} else {
    render_errors($errors);
}

Pretty clean so far! Note that we could replace the if (empty($var)) statements with a for-loop. However, I think that is overkill in this situation.

As a side note, please remember that this code is vulnerable to SQL injection. Fixing that issue is beyond the scope of the question.

Community
  • 1
  • 1
Pieter van den Ham
  • 4,381
  • 3
  • 26
  • 41
0

Less spaghetti? Start with functional decomposition, then work towards separating the task of sanitation from that of validation. I will leave out many steps that I take (such as verifying the form / $_POST / filter_input_array() has the correct number of inputs, and the correct keys are in the $_POST superglobal / INPUT_POST, etc, you might want to think about that.). Alter some of my techniques for your exact needs. Your program should be less spaghetti afterwards. :-)

Sanitize then validate. You have to keep them separated, so to speak. ;-)

Sanitizing with Functional Decomposition

Make a single task its own block of code.

If all of your sanitization steps (trim(), strip_tags(), etc.) are the same for all of your form fields, then make a sanitizer function to do that work. Note, that the one-time way you are trimming and stripping tags can be improved upon simply by using a loop. Save the original value in a variable, then trim(), strip_tags(), etc within a while loop. Compare the results to the original. If they are the same, break. If they differ, save the current value of the form field in your variable again and let the loop run again.

function sanitize($formValue)
{
    $oldValue = $formValue;

    do
    {
        $formValue = trim($formValue);
        $formValue = strip_tags($formValue);

        //Anything else you want to do.

        $formValue = trim($formValue);

        if($formValue === $oldValue)
        {
            break;
        }

        $oldValue = $formValue;
    }
    while(1); //Infinite loop

    return $formValue;
}

Then, simply run this function in a loop.

$sanitized = [];

foreach($_POST as $key => $value)
{
    $sanitized[$key] = sanitize($value);
}

/* You can keep track your variable anyway you want.*/

Looking further down the road, it is times like this where devising an input source ($_POST, $_GET, $_SESSION, $_FILES, $_COOKIE, etc..) based sanitizing, class hierarcy really comes in handy. Moreover, basing that class hierarchy on the use of filter_input_array() really puts you a head of the game. What about validation?

Validating with Functional Decomposition

You could look at each form field as needing its own validating function. Then, only the logic required to check one form field will be contained within the block. The key, retain your Boolean logic by having the validator functions return the results of a test (true / false).

function uname($uname, &$error)
{
    if(! /* Some test */)
    {
        $error = 'Totally wrong!'
    }
    elseif(! /* Another test */)
    {
        $error = 'Incredibly wrong!'
    }
    else
    { 
        $error = NULL;
    }

    return !isset($error) //If error is set, then the test has failed.
}

function email($email, &$error)
{
    if(! /* Some test */)
    {
        $error = 'Totally wrong!'
    }
    elseif(! /* Another test */)
    {
        $error = 'Incredibly wrong!'
    }
    else
    { 
        $error = NULL;
    }

    return !isset($error) //If error is set, then the test has failed.
}

function pass($pass, &$error)
{
    if(! /* Some test */)
    {
        $error = 'Totally wrong!'
    }
    elseif(! /* Another test */)
    {
        $error = 'Incredibly wrong!'
    }
    else
    { 
        $error = NULL;
    }

    return !isset($error) //If error is set, then the test has failed.
}

function passCon($passCon, &$error)
{
    if(! /* Some test */)
    {
        $error = 'Totally wrong!'
    }
    elseif(! /* Another test */)
    {
        $error = 'Incredibly wrong!'
    }
    else
    { 
        $error = NULL;
    }

    return !isset($error) //If error is set, then the test has failed.
}

In PHP, you can use variable functions to name your function the same as the fields they are checking. So, to execute these validators, simply do this.

$errorMsgs = [];

foreach($sanitized as $key => $value)
{
    $key($value, $errorMsgs[$key])
}

Then, generally speaking, you just need to see if there are any errors in the $errorMsgs array. Do this by processing the $errorMsgs array

$error = false;

foreach($errorMsgs as $key => $value)
{
    if(isset($value))
    {
         //There is an error in the $key field
         $error = true;
    }
}


..and then.

if($error === true)
{
     //Prompt user in some way and terminate processing.
}

// Send email, login, etc ....

Taken further, you could create a generic, Validator, super class.

All this being said. I do all of my sanitization and validation in an object oriented way to reduce code duplication. The Sanitizer super class has children (PostSanitizer, GetSanitizer, ....). The Validator super class has all the test one might perform on a string, integer, or float. Children of the Validator superclass are page/form specific. But, when something like a form token is needed, it's validating method is found in the Validator super-class because it can be used on any form.

A good validation routine keeps track of:

1) Input values in an associative array..

2) Test results (Booleans) in an associative array. Test results (true/false) can be converted to CSS classes or a JSON string of '1's and '0's.

3) Error messages in an associative array.

..then makes final decisions about what to do with the input values and/or error messages based on the test results (by key). If there are errors (false values in the a hypothetical test results array), use the error messages that have the corresponding key.

My previous example condenses the final error checking and error message data structures with one array, but using separate data structures allows more flexibility (decouples error messages from the detected errors). Simply store the results of each validating variable function into a $testResults array like this.

function sanitize($formValue)
{
    $oldValue = $formValue;

    do
    {
        $formValue = trim($formValue);
        $formValue = strip_tags($formValue);

        //Anything else you want to do.

        $formValue = trim($formValue);

        if($formValue === $oldValue)
        {
            break;
        }

        $oldValue = $formValue;
    }
    while(1); //Infinite loop

    return $formValue;
}

$sanitized = [];

foreach($_POST as $key => $value)
{
    $sanitized[$key] = sanitize($value);
}

$testResults = [];
$errorMsgs = [];

foreach($sanitized as $key => $value)
{
    $testResults[$key] = $key($value, $errorMsgs[$key])
}

if(!in_array(false, $testResults, true))
{
     return true  //Assuming that, ultimately, you need to know if everything worked or not, and will take action on this elsewhere. It's up to you to make the correct functions/methods, but this general foundation can get you going.
}

return false; //Obviously. Do not submit the form. Show the errors (CSS and error messages).

Then, simply check for the existence of false in the $testResults array. Get the corresponding error message from $errorMsgs using the appropriate $key. Using this generic, final stub, you can create a powerful santization and validation routine, especially if you go object oriented.

Eventually, you will begin to see that the same kinds of test are being repeated among the various validation variable functions: data type, length, regular expression, exact matches, must be a value within a set, etc. Thus, the primary difference between the validating variable functions will be the minimum and maximum string lengths, regex patterns, etc... If you are savvy, you can create an associative array that is used to "program" each variable function with its set of validation parameters. That's getting a bit beyond the scope, but that is what I do.

Thus, all my variable functions perform the same basic tests via factored out logic using a method of class Validator called validateInput(). This method receives the following arguments

1) The value to be tested. 2) An associative array of the test parameters (which can specify datatype) 3) An array element, passed in as a variable (by reference), that corresponds the field being tested that will hold the error message, if any.

What's funny, is that I use a two step sanitization and a two step validation. I use a custom filter algorithm using PHP functions, then I use the PECL filter functions (filter_input_array()). If anything fails during these steps, I throw a SecurityException (because I extend RuntimeException).

Only after these filters pass do I attempt to use the PHP/PECL filter valiation functions. Then, I run my own validation routine using validating, variable functions. Yes, these only run if the previous test passed as true (to avoid overwriting previous failures and corresponding error message).

This is entirely object oriented. Hope I helped.

Anthony Rutledge
  • 6,980
  • 2
  • 39
  • 44