0
\$\begingroup\$

Can you please tell me how to improve this code?

<?php
 include 'configdb.php';

 if(($_SERVER['REQUEST_METHOD'] === 'POST') && !empty($_POST['email']) && !empty($_POST['password']))
 {
    $getUserData = getUserByEmail();
    if (mysqli_stmt_num_rows($getUserData) > 0)
    {
        die(
          '{
            "status":"1",
            "message":"Email Already Exists"    
           }'
        );
    }
    else
    {
        insertUser();
        die('{
              "status":"0",
              "message":"Registration Successful"
          }');
    }
 }

  function insertUser()
  {
    $email = $_POST['email'];
    $password = password_hash($_POST['password'], PASSWORD_DEFAULT);

    $insertData = mysqli_prepare($GLOBALS['connection'], "INSERT INTO users (emailID, password, com_code) VALUES(?,?,?)");

    if(false == $insertData)
    {
        handlePrepareStmtErrors("-1");
    }
    $bindParam = mysqli_stmt_bind_param($insertData, "sss", $email, $password, $com_code);
    if(false == $bindParam)
    {
        handlePrepareStmtErrors("-2");
    }
    $executeStmt = mysqli_stmt_execute($insertData);
    if(false == $executeStmt)
    {
        handlePrepareStmtErrors("-3");
    }
   }

   function getUserByEmail()
   {
    $getUserData = mysqli_prepare($GLOBALS['connection'],"SELECT emailID FROM users WHERE emailID = ?");
    if(false == $getUserData)
    {
        handlePrepareStmtErrors("-1");
    }
    $bindParam = mysqli_stmt_bind_param($getUserData,"s",$_POST['email']);
    if(false == $bindParam)
    {
        handlePrepareStmtErrors("-2");
    }
    $executeStmt = mysqli_stmt_execute($getUserData);
    if(false == $executeStmt)
    {
        handlePrepareStmtErrors("-3");  
    }
    mysqli_stmt_store_result($getUserData);/*It Is Required To Get Number Of Rows By stmt_num_rows Method */
    return $getUserData;
   }

   function handlePrepareStmtErrors($code)
   {
    switch((String)$code)
    {
        case "-1":
            die('{
                "status":"-1",
                "message":"Statement prepare error"
            }');

        case "-2":
            die('{
                "status":"-1",
                "message":"Parameter binding error"
            }');
        case "-3":
            die('{
                "status":"-1",
                "message":"Statement execution error"
            }');
      }
    }
   ?>

Things about which I am not sure:

  1. I read that MySQLi prepare statements will prevent SQL injections. Is there any possibility to hack my db by my design?
  2. I am planning to create a utils.php and will add handlePrepareStmtErrors function there. In that function, I am exiting the scripts if any errors are there. So only I am not using break statement there because it will not get executed. Is it right way?
  3. I feel like my functions are not properly written, but it does what I want. I tried and registration is successful.
\$\endgroup\$
  • \$\begingroup\$ I would not use die() to send the script, you should return something useful that you can then work with else where, There is also no validation of the users input, meaning i could have a single letter user name and pass word, or random special characters \$\endgroup\$ – Josh Kirkpatrick Aug 6 '15 at 10:28
  • \$\begingroup\$ @JoshKirkpatrick I have client side validation. Doesn't enough? \$\endgroup\$ – Gibbs Aug 9 '15 at 14:59
  • \$\begingroup\$ I would have both, at least if something slips past the client side you can catch it before it hits the database \$\endgroup\$ – Josh Kirkpatrick Aug 10 '15 at 7:05
1
\$\begingroup\$

I read that MySQLi prepare statements will prevent SQL injections. Is there any possibility to hack my db by my design?

No, there shouldn't be a way. Prepared statements are the recommended way to prevent SQL injection, and you are using them correctly and consistently.

I am planning to create a utils.php and will add handlePrepareStmtErrors function there. In that function, I am exiting the scripts if any errors are there. So only I am not using break statement there because it will not get executed. Is it right way?

Yes, you don't need break if you are exiting or returning. Having a default is still a good idea though.

But you shouldn't exit. Helper functions and classes in general should not exit/die, they should throw exceptions or return false. If you just die, the calling function has no change to recover, thus making the function hard to reuse.

I feel like my functions are not properly written, but it does what I want. I tried and registration is successful.

I think that your feeling is correct.

I could for example imagine multiple usages for the function getUserByEmail, but the way you are writing it, it can't really be reused, as it needs the POST value email to be set.

Ideally, the signature would look like this: getUserByEmail($email). I also just noticed that it doesn't actually get the user, so doesUserExist($email) which then returns true or false would be more fitting.

\$\endgroup\$
  • \$\begingroup\$ Again !! I read the same suggestion of your second answer before posting the answer. Yes, I didn't think to reuse the third point. I can do that. Thanks. \$\endgroup\$ – Gibbs Aug 3 '15 at 15:28

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.