5
\$\begingroup\$

Is this correct coding for prepared statements in php. I've done some reading but the best way to learn is criticism from peers. Any suggestions to improve this code? Anything I've done wrong? Thanks you!

$dbh = new PDO('mysql:host=localhost;dbname=tests;charset=UTF-8', $user, $pass);
     $stmt = $dbh->prepare('INSERT INTO items (item_id,itemname,itemuni, soldby,soldby2,keywords,description,sizing,price,totalquantity,lowquantity,weight,length,width,height,imagelocation,thumblocation,fblocation,cat1,cat2,cat3,size1,quantity1,size2,quantity2,size3,quantity3,size4,quantity4,size5,quantity5,size6,quantity6,size7,quantity7,size8,quantity8,size9,quantity9,size10,quantity10,size11,quantity11,size12,quantity12,size13,quantity13) VALUES (:item_id,:itemname,:itemuni,:soldby,:soldby2,:keywords,:description,:sizing,:price,:totalquantity,:lowquantity,:weight,:length,:width,:height,:imagelocation,:thumblocation,:fblocation,:cat1,:cat2,:cat3,:size1,:quantity1,:size2,:quantity2,:size3,:quantity3,:size4,:quantity4,:size5,:quantity5,:size6,:quantity6,:size7,:quantity7,:size8,:quantity8,:size9,:quantity9,:size10,:quantity10,:size11,:quantity11,:size12,:quantity12,:size13,:quantity13)');
$dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
    $stmt->bindValue(':item_id', '44', PDO::PARAM_STR);
    $stmt->bindValue(':itemname', $_POST['itemname'], PDO::PARAM_STR);
    $stmt->bindValue(':itemuni',$getitemuni, PDO::PARAM_STR);
    $stmt->bindValue(':soldby', $soldby, PDO::PARAM_STR);
    $stmt->bindValue(':solby2', $soldby2, PDO::PARAM_STR);
    $stmt->bindValue(':keywords', $_POST['keywords'], PDO::PARAM_STR);
    $stmt->bindValue(':description', $_POST['description'], PDO::PARAM_STR);
    $stmt->bindValue(':sizing', 'yes', PDO::PARAM_STR);
    $stmt->bindValue(':price', $_POST['price'], PDO::PARAM_STR);
    $stmt->bindValue(':totalquantity', $_POST['quantity'], PDO::PARAM_STR);
    $stmt->bindValue(':lowquantity', $_POST['lowquantity'], PDO::PARAM_STR);
    $stmt->bindValue(':weight', $_POST['weight'], PDO::PARAM_STR);
    $stmt->bindValue(':length', $_POST['length'], PDO::PARAM_STR);
    $stmt->bindValue(':width', $_POST['width'], PDO::PARAM_STR);
    $stmt->bindValue(':height', $_POST['height'], PDO::PARAM_STR);
    $stmt->bindValue(':imagelocation', $_POST['imageloc'], PDO::PARAM_STR);
    $stmt->bindValue(':thumblocation', $_POST['thumbloc'], PDO::PARAM_STR);
    $stmt->bindValue(':fblocation', $_POST['fbloc'], PDO::PARAM_STR);
    $stmt->bindValue(':cat1', $_POST['drop_1'], PDO::PARAM_STR);
    $stmt->bindValue(':cat2', $_POST['drop_2'], PDO::PARAM_STR);
    $stmt->bindValue(':cat3', $_POST['drop_3'], PDO::PARAM_STR);
    $stmt->bindValue(':size1', $_POST['size1'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity1', $_POST['quant1'], PDO::PARAM_STR);
    $stmt->bindValue(':size2', $_POST['size2'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity2', $_POST['quant2'], PDO::PARAM_STR);
    $stmt->bindValue(':size3', $_POST['size3'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity3', $_POST['quant3'], PDO::PARAM_STR);
    $stmt->bindValue(':size4', $_POST['size4'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity4', $_POST['quant4'], PDO::PARAM_STR);
    $stmt->bindValue(':size5', $_POST['size5'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity5', $_POST['quant5'], PDO::PARAM_STR);
    $stmt->bindValue(':size6', $_POST['size6'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity6', $_POST['quant6'], PDO::PARAM_STR);
    $stmt->bindValue(':size7', $_POST['size7'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity7', $_POST['quant7'], PDO::PARAM_STR);
    $stmt->bindValue(':size8', $_POST['size8'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity8', $_POST['quant8'], PDO::PARAM_STR);
    $stmt->bindValue(':size9', $_POST['size9'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity9', $_POST['quant9'], PDO::PARAM_STR);
    $stmt->bindValue(':size10', $_POST['size10'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity10', $_POST['quant10'], PDO::PARAM_STR);
    $stmt->bindValue(':size11', $_POST['size11'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity11', $_POST['quant11'], PDO::PARAM_STR);
    $stmt->bindValue(':size12', $_POST['size12'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity12', $_POST['quant12'], PDO::PARAM_STR);
    $stmt->bindValue(':size13', $_POST['size13'], PDO::PARAM_STR);
    $stmt->bindValue(':quantity13', $_POST['quant13'], PDO::PARAM_STR);
    $stmt->execute()
\$\endgroup\$
6
\$\begingroup\$

First off, if I were about to do something this repetitive, I would set it up so that I would only have to type it all once. Best way to do this is set up an array and run it through loops. Lets start with that array.

$bindValues = array(
    'item_id' => '44',
    'itemuni' => $getitemuni,
    'soldby' => $soldby,
    etc...
);

"Wait a second! You skipped some elements and those keys are missing their colons!" Yep, DON'T add post data yet as it needs to be sanitized first. You should ALWAYS sanitize user input. Also, do not append a colon(:) to any of these array keys. You'll see why in a bit, I promise.

Now, I'm assuming you are using the entire post array. If you are not, you should either use what you need first, or set those values to a new array/variable and then unset them from the post array. Here's what you are going to do with your post array.

$filterType = array(
    'string' => FILTER_SANITIZE_STRING | FILTER_SANITIZE_STRIPPED,
    'integer' => FILTER_VALIDATE_INT,
    etc...
);

$post = array();
foreach($_POST as $key => $value) {
    $filter = $filterType[gettype($value)];
    $value = filter_var($value, $filter);
    $post[$key] => $value;
}
$bindValues = array_merge($bindValues, $post);

Here's a little explanation of what the above code does. $filterType sets up an array that will hold specific flags needed for PHP's filter_var() function. See the documentation for an explanation of this function and its flags. $post sets up a new array to add our sanitized $_POST data to. Enter the loop. $filter uses PHP's gettype() function to return the $value's type and compare it to the $filterType array for the necessary flags to use in the filter_var() function. $value is the sanitized value that you want to save to the new array. Using this new information we take the original key and sanitized value and append it to the new array. Exit loop. Merge both arrays.

Now that we are done with setup, you'll want to start the code that actually does something.

$items = implode(',', array_keys($bindValues));
$values = implode(',:', array_keys($bindValues));
/*
var_dump() these variables if you want to examine them.
Basically, this just replaces those long SQL strings you had
*/

$stmt = $dbh->prepare("INSERT INTO items ($items) VALUES (:$values)");

foreach($bindValues as $key => $value) {
    $stmt->bindValue(':' . $key, $value, PDO::PARAM_STR);
}

$stmt->execute();

Sorry if you were looking for a little more generic answer. I got a little carried away.

\$\endgroup\$
  • \$\begingroup\$ no thank you!!! This is perfect i wanted detail i love it! I love to learn what is wrong and how to fix it! Thank you again! \$\endgroup\$ – Easley Apr 25 '12 at 4:19
3
\$\begingroup\$

I agree with all of showerhead's comments.

You should make the following three lines from your code the first ones:

$dbh = new PDO('mysql:host=localhost;dbname=tests;charset=UTF-8', $user, $pass);
$dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

Then do all of the stmt lines as you had them. If you don't set the error mode attribute to PDO::ERRMODE_EXCEPTION before doing your prepare then it will not throw an exception on failure to prepare your statement. So as you have it now your prepare could produce $stmt as false.

Suggestions

  • Limit your lines to a set number of characters. I like to use 80 characters. Monitors can easily handle that without wrapping. Side-by-side diffs are also easily readable.
  • There are too many fields in your database table!
  • You have fields that couldn't possibly be filled for all items: quantity11, quantity12 (surely not everything has that many).
  • Fields like totalquantity, lowquantity, length, width and many others should be stored in a numeric form within your database. This would also mean that you wouldn't bind them with PDO::PARAM_STR
  • Your fields don't have meaningful names (quantity11?)

I think the next step for you would be to learn about how to make a good database. Look for articles on Database Normalization and Database Design. This will help you break this super table into parts. It will help when you want to find information from your database later.

\$\endgroup\$
  • \$\begingroup\$ I'm ashamed to say I have no real experience with SQL, so I was not able to provide those other suggestions, but they look in order. I know SQL is every PHP programmers bread and butter, but everything I've done thus far has been XML based. My next big project will fix that lack. Also agree with everything he has here +1 \$\endgroup\$ – mseancole Apr 25 '12 at 4:01
  • \$\begingroup\$ I'd swear, if I didn't know any better, I'd say we were following each other around ;) \$\endgroup\$ – mseancole Apr 25 '12 at 4:02
  • \$\begingroup\$ @paul thank you!!! I really appreciate your time into the answer. I don't want to bother you any further but I love new reading material. If you have any suggestions on what to read for Design and Normalization I would love to read it! Thanks again! \$\endgroup\$ – Easley Apr 25 '12 at 4:21
  • \$\begingroup\$ BTW I wish I could accept both answers. They are both terrific!!! \$\endgroup\$ – Easley Apr 25 '12 at 4:31
  • \$\begingroup\$ @showerhead Yes, nice to have you reviewing PHP too. I was going to miss out your good points with my answer. \$\endgroup\$ – Paul Apr 25 '12 at 4:31

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.