3
\$\begingroup\$

I'm just starting out using OOP; classes and methods etc. I've been looking around for some PDO classes/wrappers out there, but there's not much to choose from. So I've tried to make my own. Started out by writing an "insert" method.

As a man with low self esteem, I never like what I do myself, so I thought I'd ask you guys here for feedback. Here it is:

public function insert($tbl, $data) {

    $this->_stmt = $this->_dbh->prepare("INSERT INTO $tbl (" . implode(', ', array_keys($data)) . ") VALUES (:" . implode(', :', array_keys($data)) . ")");

    foreach($data as $key => $value) {
        $this->_stmt->bindValue($key, $value, PDO::PARAM_STR);
    }

    $this->_stmt->execute();
}

The meaning was that it could all be done in one sweep instead of having several methods to do one thing; inserting a record.

An example of using this code:

$message   = 'A message for the ones who like to read it!';
$sent_on   = 'Saturday 23rd 2012';
$unique_id = 'unique_as_can_get';

$data = array(
    'message'   => $message,
    'sent_on'   => $sent_on,
    'unique_id' => $unique_id
);

$insert = new DB;
$insert->insert('tablename', $data);

I've put the database connection into the constructor.

The code works as I want it to, but I'm still confused if it's "accepted" amongst you people who are way more skilled that I will ever be.

Please let me know if this code if "usable" and/or what is wrong with it, what can be improved/changed etc.

\$\endgroup\$
3
\$\begingroup\$

There's nothing wrong with it, but it's a small piece of code. Some minor details are:

1: I would write $table instead of $tbl, why abbreviate it? You also write foreach ($data as $key => $value), which is very general. Why not specify it better: `foreach ($row as $column => $value)'? What I mean is that variable names should have meaning. I know an array has keys and values, but they are general names. Here you should use names that tell you what a variable really represents.

2: $this->_stmt is a class variable, where a local $statement variable would do. Local variables are always more efficient and have even better encapsulation.

3: Also watch your line length: 156 characters is too much. Instead of writing this:

$this->_stmt = $this->_dbh->prepare("INSERT INTO $tbl (" . implode(', ', 
array_keys($data)) . ") VALUES (:" .  implode(', :', array_keys($data)) . ")");

(I wrapped it for clarity), you could have written something like;

$rowkeys   = array_keys($row); 
$columns   = implode(',',$rowkeys);
$values    = ':'.implode(',:',$rowkeys);
$query     = "INSERT INTO $table ($columns) VALUES($values)";
$statement = $this->handle->prepare($query);

This makes it easier to read, and debug. It may seem longer, but effectively it does the same thing.

4: You could build in error checks. Is the array a valid array to insert? Start with is_array() for instance. Do the column names exist in the table? Does the insert execute properly? No errors?

5: you could return the lastInsertID(): http://php.net/manual/en/pdo.lastinsertid.php

6: You cannot insert multiple rows at once with this method. Perhaps you don't need this, but if you do you could add that functionallity.

7: You bind values, so that's quite secure. However, are you sure your column names can never be influenced by outside sources (= hackers)? Another good reason to check them, because they go straight into your SQL command.

8: Please note that wrapping PDO can be a burden later. See: Class for reducing development time If that puts you complete off, don't worry, I also use a wrapper despite all that good advice.

\$\endgroup\$
1
  • \$\begingroup\$ KIKO Software: Thank you very much! I will take your advices and use them to make it better. Note that this is just the "insert" method for one record; not the entire class. I will take use of transactions in another method if several records should be inserted :) Regards Fredo \$\endgroup\$ – FredoSk Mar 16 '15 at 15:04

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.