Class method to insert a record into MySQL

Question

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.

Answer 1

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.