Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

##Is it good practice to insert data into a table this way?

Is it good practice to insert data into a table this way?

###Prepared statements are reusable

Prepared statements are reusable

##Is it good practice to insert data into a table this way?

###Prepared statements are reusable

Is it good practice to insert data into a table this way?

Prepared statements are reusable

added 1768 characters in body
Source Link

Other niggles:

Your usage of this (badly) named $array variable doesn't really make sense:

$array = array();
$array['table'] = 'users';
$array['field_names'] = $field_names;
$array['alias'] = $alias;
$array['assignment'] = $assignment;

For starters, initializing a variable to an empty array, and then adding keys one by one is a lot more work (to type) than a simple:

$array = [
 'table' => 'users',
 'field_names' => $field_names,
 'alias' => $alias,
 'assignment' => $assignment,
];

Now if you were to pre-format/stringify array values (like $field_names), you could use vsprintf to create your query, but that's something others who work on the same code will hate you for (they really will, trust me, I've been there).
I've already hinted at the possibility of creating a class, which could be used to store things that are, essentially, immutable (like the table name and the field map). But that's a different matter, for now: just start by leaving out the $array variable. There's also no need to assign the format to $q if you're reassigning $q the next statement. Just write:

$q = sprintf(
 'INSERT INTO `%s` (%s) VALUES (%s)',
 'users',
 implode(', ', $field_names),
 implode(', ', $alias)
);

The backticks around the table name imply you're using MySQL, you might want to add the same backticks around your field names (in case some mug decides to call fields "transaction" or "set" - again, I've been there, it happens):

$q = sprintf(
 'INSERT INTO `%s` (`%s`) VALUES (%s)',
 'users',
 implode('`, `', $field_names),
 implode(', ', $alias)
);

Other niggles:

Your usage of this (badly) named $array variable doesn't really make sense:

$array = array();
$array['table'] = 'users';
$array['field_names'] = $field_names;
$array['alias'] = $alias;
$array['assignment'] = $assignment;

For starters, initializing a variable to an empty array, and then adding keys one by one is a lot more work (to type) than a simple:

$array = [
 'table' => 'users',
 'field_names' => $field_names,
 'alias' => $alias,
 'assignment' => $assignment,
];

Now if you were to pre-format/stringify array values (like $field_names), you could use vsprintf to create your query, but that's something others who work on the same code will hate you for (they really will, trust me, I've been there).
I've already hinted at the possibility of creating a class, which could be used to store things that are, essentially, immutable (like the table name and the field map). But that's a different matter, for now: just start by leaving out the $array variable. There's also no need to assign the format to $q if you're reassigning $q the next statement. Just write:

$q = sprintf(
 'INSERT INTO `%s` (%s) VALUES (%s)',
 'users',
 implode(', ', $field_names),
 implode(', ', $alias)
);

The backticks around the table name imply you're using MySQL, you might want to add the same backticks around your field names (in case some mug decides to call fields "transaction" or "set" - again, I've been there, it happens):

$q = sprintf(
 'INSERT INTO `%s` (`%s`) VALUES (%s)',
 'users',
 implode('`, `', $field_names),
 implode(', ', $alias)
);
added 1463 characters in body
Source Link

###Prepared statements are reusable

Another drawback of the way you're using prepared statements in your function is that you're missing out on one of their biggest advantages: prepared statements can be re-used several times:

$stmt = $pdo->prepare('INSERT INTO foo (field1, field2) VALUES (:foo, :bar)');
$stmt->execute([':foo' => 'value 1', ':bar' => 1]);
$stmt->execute([':foo' => 'value 2', ':bar' => 2);

The code above will add 2 rows to the foo table, using the same prepared statement. Creating such a statement in a function, using it and then returning a boolean will GC (garbage collect) the prepared statement, only to create it a second time after when you call the function again. This isn't ideal, obviously.

It might be more useful to harness the power of the PDOStatement instance that PDO::prepare returns.
This can be especially useful if you're using SELECT queries: returning the PDOStatement instance allows the user to fetch the data in a format he sees fit:

$stmt = $obj->getSelectByIdStmt();
$data = [];
foreach ($ids as $id) {
 $stmt->execute([':id' => $id]);
 $objects = [];
 //or PDO::FETCH_ASSOC, PDO::FETCH_CLASS, ...
 while ($row = $stmt->fetch(PDO::FETCH_OBJ) {
 $objects[] = $row;
 }
 $data[$id] = $objects;
}

This is something that is quite hard to abstract into one, comprehensive function/method...

###Prepared statements are reusable

Another drawback of the way you're using prepared statements in your function is that you're missing out on one of their biggest advantages: prepared statements can be re-used several times:

$stmt = $pdo->prepare('INSERT INTO foo (field1, field2) VALUES (:foo, :bar)');
$stmt->execute([':foo' => 'value 1', ':bar' => 1]);
$stmt->execute([':foo' => 'value 2', ':bar' => 2);

The code above will add 2 rows to the foo table, using the same prepared statement. Creating such a statement in a function, using it and then returning a boolean will GC (garbage collect) the prepared statement, only to create it a second time after when you call the function again. This isn't ideal, obviously.

It might be more useful to harness the power of the PDOStatement instance that PDO::prepare returns.
This can be especially useful if you're using SELECT queries: returning the PDOStatement instance allows the user to fetch the data in a format he sees fit:

$stmt = $obj->getSelectByIdStmt();
$data = [];
foreach ($ids as $id) {
 $stmt->execute([':id' => $id]);
 $objects = [];
 //or PDO::FETCH_ASSOC, PDO::FETCH_CLASS, ...
 while ($row = $stmt->fetch(PDO::FETCH_OBJ) {
 $objects[] = $row;
 }
 $data[$id] = $objects;
}

This is something that is quite hard to abstract into one, comprehensive function/method...

Source Link
Loading
lang-php

AltStyle によって変換されたページ (->オリジナル) /