2
\$\begingroup\$

I'm working on creating a user registration script in PHP. I have two tables that need to be inserted into. One that stores some general user information, and another that stores their login credentials. My issue is that my code to perform these insert is a mess of if/else blocks and I'm trying to figure out a good way to condense this down into a logical and clean code block.

Note that for now I have just placed some echo's that will later be replaced by a real error handling system once I truly know how many paths I'm going to need.

Can someone please review this script and let me know some improvements I can make to it? I know it's quite ugly right now.

$mysqli = getMysqlConnection();
  if ($stmt = $mysqli->prepare("INSERT INTO users (username, email, regtime, emailverified, type) values (?,?,?,?,?);")) {
      date_default_timezone_set("America/New_York");
      $dateStr = date("m-d-Y h:i:s");
      $emailverified = 0;
      $type = 0;
      $rc = $stmt->bind_param('sssii', $username, $email, $dateStr, $emailverified, $type);
      if($rc === true){
        $rc = $stmt->execute();
        if($rc === true)
        {
          if ($stmt = $mysqli->prepare("INSERT INTO usercreds (username, hash) VALUES (?, ?);")) {
              $rc = $stmt->bind_param('ss', $username, $hash);
              if($rc === true){
                $rc = $stmt->execute();
                if($rc !== true)
                {
                  echo "so close...";
                }
              }
              else
              {
                echo "well, shit...";
              }
          }
          else {
            {
              echo "whoops...";
            }
          }
        }
        else {
          echo "noooooooooo!";
        }
      }
  }
  else {
    echo "uh oh";
  }

  echo "done";
\$\endgroup\$
2
  • \$\begingroup\$ And these echo messages serve what purpose exactly? \$\endgroup\$ – Mathieu Guindon May 13 '16 at 3:43
  • \$\begingroup\$ Exceptions can be your friend. Switch to a pdo based approach configured to throw exceptions and get rid of all those silly rc statements. All they do add code without any useful benefits. In fact, switch to the Doctrine database access layer library and get rid of all those bind statements as well. \$\endgroup\$ – Cerad May 14 '16 at 17:05
1
\$\begingroup\$

You should return early to reduce nesting and make it more obvious when what values are returned:

if (!$stmt = $mysqli->prepare("INSERT INTO users (username, email, regtime, emailverified, type) values (?,?,?,?,?);")) {
    echo "uh oh";
}
[...]

if($stmt->bind_param('sssii', $username, $email, $dateStr, $emailverified, $type) !== true){
    // you actually didn't have an else for this
}
[etc]

You should also replace your echoes with exceptions with proper error messages.

Misc

  • your indentation is off, and you should use a minimum of 4 spaces.
  • rc is not a good variable name, and you shouldn't reuse variables for different things.
  • you should also extract this into two functions: insertUser($username, $email, ...) and insertUserCredentials($username, $hash).
\$\endgroup\$
0
\$\begingroup\$

Briefly, I would at least think about organisation ...

<?php
date_default_timezone_set("America/New_York");

function updateUser($connection, $username, $email, $regTime, $isVerified, $type) {
    $stmt = $connection->prepare("INSERT INTO users (username, email, regtime, emailverified, type) values (?,?,?,?,?);");
    $rc = $stmt->bind_param('sssii', $username, $email, $dateStr, $emailverified, $type);
    $rc = $stmt->execute();

    return $rc ? true : false;
}

function updateCredentials($username, $hash) {
    $stmt = $mysqli->prepare("INSERT INTO usercreds (username, hash) VALUES (?, ?);");
    $rc = $stmt->bind_param('ss', $username, $hash);
    $rc = $stmt->execute();

    return $rc ? true : false;
}

$username = 'someUserName';
$dateStr = date("m-d-Y h:i:s");
$emailverified = 0;
$type = 0;

$mysqli = getMysqlConnection();

$updatedUser = updateUser($mysqli, $username, $email, $dateStr, $emailVerified, $type);
if (!$updatedUser) {
    throw new \Exception("There was a problem updating the user:" . $username);
}

$updatedCreds = updateCredentials($mysqli, $hash);
if (!$updatedUser) {
    throw new \Exception("There was a problem updating the credentials:" . $username);
}

echo "done";
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.