patternphpMinor
Class method to insert a record into MySQL
Viewed 0 times
insertmethodintorecordmysqlclass
Problem
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:
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:
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.
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.
Solution
There's nothing wrong with it, but it's a small piece of code. Some minor details are:
1: I would write
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.
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.php6: 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.
Code Snippets
$this->_stmt = $this->_dbh->prepare("INSERT INTO $tbl (" . implode(', ',
array_keys($data)) . ") VALUES (:" . implode(', :', array_keys($data)) . ")");$rowkeys = array_keys($row);
$columns = implode(',',$rowkeys);
$values = ':'.implode(',:',$rowkeys);
$query = "INSERT INTO $table ($columns) VALUES($values)";
$statement = $this->handle->prepare($query);Context
StackExchange Code Review Q#84171, answer score: 3
Revisions (0)
No revisions yet.