Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm currently changing some legacy code over to PDO. The below code works, but not 100%. I would like to know if it's best practice or if anything can be done better.

//Database Array
$config['db'] = array(
    'host' => 'localhost',
    'username' => 'root',
    'password' => 'root',
    'dbname' => 'local');

//New PDO
$db = new PDO('mysql:host=' . $config['db']['host'] . ';dbname=' . $config['db']['dbname'], $config['db']['username'], $config['db']['password']);
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
//Check connection is ok
try {
    $db->exec("SET CHARACTER SET utf8");
}
catch (PDOException $ex) {
    print "Error!: " . $ex->getMessage() . "<br/>";
    die();
}

//Update users function
function update($db, $fn, $ln, $email, $offers, $vlue, $response)
{
    $stmt = $db->prepare("insert into local (fName_765, lName_765, email_765, signup_765) values (:fname, :lname, :email, :signup, NOW())");
    $stmt->bindParam(':fname', $fn, PDO::PARAM_STR);
    $stmt->bindParam(':lname', $ln, PDO::PARAM_STR);
    $stmt->bindParam(':email', $email, PDO::PARAM_STR);
    $stmt->bindParam(':signup', $offers, PDO::PARAM_STR);
    try {
        $stmt->execute();
        print $db->lastInsertId(); //show ID
        return true;
    }
    catch (PDOException $e) {
        print "Error!: " . $e->getMessage() . "<br/>"; // show error
        return false;
    }
}

//Test Attributes
$fn = 'test';
$ln = 'test';
$email = 'tesst@test,com';
$offers = '1';

if (!update($db, $fn, $ln, $email, $offers, $vlue, $response)) {
    echo "no update there is a slight problem";
} else {
    echo "it seemed to work";
}

Midified version:

$functionError = array();

//Database Array
$config['db'] = array(
    'host' => 'localhost',
    'username' => 'root',
    'password' => 'root',
    'dbname' => 'root');

//Check connection is ok
try {
    $db = new PDO('mysql:host=' . $config['db']['host'] . ';dbname=' . $config['db']['dbname'], $config['db']['username'], $config['db']['password']);
    $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}
catch (PDOException $e) {
    $functionError[] = "<p>{DB} - Error!: " . $e->getMessage() . "</p>";
    errorhandler ($functionError);
    die ();
}

//Error Handler
function errorhandler ($functionError) {
    print_r ($functionError);
    echo "Sorry we are experiencing issues we are aware of the problem";

}

//Update users function
function update($db, $fn, $ln, $email, $offers, $vlue, $response)
{
    $stmt = $db->prepare("insert into kkt (fName_765, lName_765, email_765, signup_765, kkt_resp_765, kkt_respSate_765, stamp_765) values (:fname, :lname, :email, :signup, :kkt_rsp, :kkt_respState, NOW())");

    $parameters = array(
        ':fname' => $fn,
        ':lname' => $ln,
        ':email' => $email,
        ':signup' => $offers,
        ':kkt_rsp' => $vlue,
        ':kkt_respState' => $response);

    try {
        $stmt->execute($parameters);
        return true;
    }
    catch (PDOException $ex) {
        $functionError[] = "<p>{INSERT} - Error!: " . $ex->getMessage() . "</p>";
        errorhandler ($functionError);
        return false;
    }
} 

//Test Attributes
$fn = 'test';
$ln = 'test';
$email = 'tesst@test,com';
$offers = '1';
$vlue = 'value';
$response = 'resp';

if (update($db, $fn, $ln, $email, $offers, $vlue, $response)) {
    echo "it seemed to work";
    print $db->lastInsertId();
    header('Location: thank-you.php');
    exit;
}
share|improve this question
    
Please declare cross-posting, to reduce duplication of effort (from here). Normally not declaring is a downvoteable offence, but I'll resist this time! ;-) –  halfer Feb 12 '13 at 14:15
add comment

1 Answer

up vote 4 down vote accepted

Here are a few things I noticed.

  1. If you are going to catch a possible exception when you connect to your MySQL DB you want to include the PDO instantiation in the try block.

    try {  
        $db = new PDO('mysql:host=' . $config['db']['host'] . ';dbname=' . $config['db'}['dbname'], $config['db']['username'], $config['db']['password']);
        $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        $db->exec("SET CHARACTER SET utf8");
    }  catch(PDOException $e) {  
        print "Error!: " . $ex->getMessage() . "<br/>";
        die(); 
    }
    
  2. The arguments $vlue and $response don't do anything in your update function. You probably want to get rid of those.

  3. You can use a quicker syntax for binding parameters by defining an array that holds the values and passing it to PDOStatement::execute(). This has the benefit of less function calls. Doing it this way will default all parameter value datatypes to be PDO::PARAM_STR, which matches what you are doing currently.

    $parameters = array(
        ':fname'  => $fn,
        ':lname'  => $ln,
        ':email'  => $email,
        ':signup' => $offers
    );
    $stmt->execute($parameters);
    
  4. The point of exceptions is to be able to handle, on your own terms, exceptional events. If a PDO exception occurs in your update function, more than likely it won't/shouldn't know how to handle it. It makes more sense for the code that calls update to take care of that situation. So I would suggest moving the try/catch block to wherever the update function is called.

       try {
           update($db, $fn, $ln, $email, $offers, $vlue, $response)
       } catch (PDOException $e) {
           echo "no update there is a slight problem " . $e->getMessage();
       }
    
share|improve this answer
    
thank you for your comments ill look into these asap –  odd Feb 11 '13 at 9:31
    
I have modified based on your feed back thank you, and it all makes sense. the reason behind the if(!update was i need to know if it was successful so i can carry on with the code. Is it best practice to add an if update was TRUE under my code to then run some other code. thanks in advance –  odd Feb 11 '13 at 9:54
    
update with a few more functions if you have time would be great for some feedback –  odd Feb 11 '13 at 11:59
    
PDOStatement::execute returns true on success and false on failure, so you can do something like $result = $stmt->execute($parameters), then at the end of function, return $result. –  maxiscool Feb 12 '13 at 0:16
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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