Three PHP database queries to manage accounts

Question

I have a database connection PHP file which has a lot of queries to the database. Here are 3 of the functions that I am using:

public function deleteAccount($token)
{
    $sql1 =
        "DELETE FROM $this->table_user
         WHERE $this->key_token = ?;";

    $stmt1 = $this->connection->prepare($sql1);
    $stmt1->bind_param("s", $token);
    $stmt1->execute();
    $result = $stmt1->affected_rows;
    $stmt1->close();
    return ($result >= 1);
}

public function getAccountDetails($token)
{
    $sql1 =
        "SELECT $this->key_name, $this->key_username, $this->key_email
         FROM $this->table_user
         WHERE $this->key_token = ?;";
    $stmt1 = $this->connection->prepare($sql1);
    $stmt1->bind_param("s", $token);
    $stmt1->execute();
    $stmt1->bind_result($name, $username, $email);
    $stmt1->fetch();
    $stmt1->close();
    return
        array(
            "name" => $name,
            "username" => $username,
            "email" => $email
        );
}

public function resetPassword($password, $hash, $email)
{
    $sql1 =
        "UPDATE $this->table_user
         SET $this->key_hash = ?
         WHERE $this->key_email = ? AND $this->key_passwordhash = ?;";

    $stmt1 = $this->connection->prepare($sql1);
    $passhash = password_hash($password, $this->algorithm);
    $stmt1->bind_param("sss", $passhash, $email, $hash);
    $stmt1->execute();
    $result = ($stmt1->affected_rows == 1);
    $stmt1->close();
    return $result;
}

To me this seems like a lot of code duplication and I find myself doing a lot of copying and pasting which is a big sign that it is very duplicated but I am struggling to see how I can refactor this code so that for every sql query I perform it does not require so many lines of code.

Answer 1

Common functionality refactoring

public function deleteAccount($token)
{
    $sql1 =
        "DELETE FROM $this->table_user
         WHERE $this->key_token = ?;";

    $stmt1 = $this->connection->prepare($sql1);
    $stmt1->bind_param("s", $token);
    $stmt1->execute();
    $result = $stmt1->affected_rows;
    $stmt1->close();
    return ($result >= 1);
}

You could change this to something like

private function modify($sql, array $parameters = [])
{
    $stmt = $this->connection->prepare($sql);

    if (!empty($parameters))
    {
        // ensure $parameters are passed by reference
        $parameters_by_reference = [];
        for ($i = 0, $n = count($parameters); $i < $n; $i++)
        {
            $parameters_by_reference[] =& $parameters[$i];
        }

        call_user_func_array([$stmt, bind_param], $parameters_by_reference);
    }

    $stmt->execute();
    $result = $stmt->affected_rows;
    $stmt->close();

    return $result;
}

public function deleteAccount($token)
{
    $sql = <<<EOSQL
DELETE FROM $this->table_user
  WHERE $this->key_token = ?
EOSQL;

    return ($this->modify($sql, ['s', $token]) >= 1);
}

Now it only takes two or three statements to set up the SQL query. The repeated part is all inside the modify function. Note that modify can handle any of INSERT, UPDATE, or DELETE.

I prefer the heredoc as being better for multiline strings.

I don't like numbered variables, particularly if there's only one of them.

I prefer to use single quotes for PHP strings that do not have any variable interpolation in them.

You do not need to pass a ; when executing a single statement programmatically.

Be careful about $this->table_user and $this->key_token. These should NOT contain any user input. Otherwise, this would be subject to SQL injection.

You can then change

public function resetPassword($password, $hash, $email)
{
    $sql1 =
        "UPDATE $this->table_user
         SET $this->key_hash = ?
         WHERE $this->key_email = ? AND $this->key_passwordhash = ?;";

    $stmt1 = $this->connection->prepare($sql1);
    $passhash = password_hash($password, $this->algorithm);
    $stmt1->bind_param("sss", $passhash, $email, $hash);
    $stmt1->execute();
    $result = ($stmt1->affected_rows == 1);
    $stmt1->close();
    return $result;
}

to

public function resetPassword($password, $hash, $email)
{
    $sql = <<<EOSQL
UPDATE $this->table_user
  SET $this->key_hash = ?
  WHERE $this->key_email = ? AND $this->key_passwordhash = ?
EOSQL;

    $passhash = password_hash($password, $this->algorithm);

    return ($this->modify($sql, 'sss', $passhash, $email, $hash) == 1);
}

PDO

Note that if you switch to PDO, then modify could be simpler:

private function modify($sql, array $parameters = [])
{
    $stmt = $this->connection->prepare($sql);

    foreach ($parameters as $parameter => $value)
    {
        $stmt->bindParam($parameter, $value);
    }

    $stmt->execute();

    return $stmt->rowCount();
}

And you would call it like

    return ($this->modify($sql, [1 => $passhash, 2 => $email, 3 => $hash]) == 1);
    return ($this->modify($sql, [1 => $token]) >= 1);

Or you could use named parameters, but that requires modifying the SQL string as well.

Answer 2

Consider a generalized function to run the parameterized queries and have all others pass values as arguments. Call_user_func_array() is used to handle variable list of parameters:

public function runSQL($sqlstatement, $params, $sqltype)
{   
    $stmt1 = $this->connection->prepare($sqlstatement);
    $vals = array();
    foreach($params as $key => $value) $vals[$key] = &$params[$key];    
    call_user_func_array(array($stmt, 'bind_param'), $vals);

    $stmt1->execute();

    switch ($sqltype) {                
        case "update": $result = $stmt1->affected_rows >=1 ; break;
        case "delete": $result = $stmt1->affected_rows == 1; break;
        case "select": $result = array($stmt1->bind_result($name, $username, $email)); 
                       $stmt1->fetch(); break;            
    }

    $stmt1->close();
    return($result);
}


public function deleteAccount($token)
{
    $sql1 =
        "DELETE FROM $this->table_user
         WHERE $this->key_token = ?;";

    $output = runSQL($sqli, array("s", $token), "delete");
    return ($output);
}

public function getAccountDetails($token)
{
    $sql1 =
        "SELECT $this->key_name, $this->key_username, $this->key_email
         FROM $this->table_user
         WHERE $this->key_token = ?;";

    $output = runSQL($sqli, array("s", $token), "select");
    return ($output);    
}

public function resetPassword($password, $hash, $email)
{
    $sql1 =
        "UPDATE $this->table_user
         SET $this->key_hash = ?
         WHERE $this->key_email = ? AND $this->key_passwordhash = ?;";
    $passhash = password_hash($password, $this->algorithm);

    $output = runSQL($sqli, array("sss", $passhash, $email, $hash), "update");
    return ($output);    
}

Answer 3

I honestly don't think this code is a good candidate for refactoring unless you are wanting to replace mysqli with an ORM of some sort against which you can simply make calls like account::get($id), account::delete($id), etc.

If you look at the other answers here, where refactoring is attempted, you will see examples that are (IMO at least) more complex and harder to understand than what you have now. They also don't do things like handle the various DB response which will differ greatly between SELECT, INSERT, DELETE, and UPDATE operations. You potentially need to evaluate the DB response differently for each of these query types to determine if operation was successful. If you had the proper error handling for each of these scenarios in a common function, you would get some complex conditional branching.

My main comments in reviewing this code are:

• You seem to only consider happy path. For example, what happens is statement preparation fails? What happens if the query itself fails?
• You are doing no validation of parameters in your methods. You should consider checking that values passed are as expected - non-zero length strings, integer id's etc. before attempting to query the database using these values.
• Your queries are hard to read. I really dislike the direct interpolation of variables within double quotes here as it makes it harder to understand where variables are being concatenated into your SQL. This may be just a matter of opinion but I would prefer curly braces around variables to make them more visible and backticks so it is clear to the reader that these are database object names you are specifying.

That might look something like this:

$sql1 = "
SELECT `{$this->key_name}`, `{$this->key_username}`, `{$this->key_email}`
FROM `{$this->table_user}`
WHERE `{$this->key_token}` = ?";

• You also need to be careful about SQL injection if any of the object properties used in these queries for table and field names are provided from user input (vs. being hardcoded in your app or app configuration).