Working with databases in PHP

Question

I am 100% self-taught, and so I try to read as much as I can about best practices, conventions, etc. However I freelance, and have been in a long-term contract position for the past little while with almost no peers to review my code. So I get nervous about developing bad habits.

Looking closely at this function I wrote (meant to give me a quick look over the db make sure nothing else is signed that ought to be unsigned):

$db = Database::instance();
$tables = array();
$result = $db->query('SHOW TABLES');
foreach($result as $table)
{
        $create = $db->query("SHOW CREATE TABLE `".current($table)."`");
        $tables[current($create->current())] = end($create->current());
}
echo var_dump($tables);

I see a couple things - $tables and $table are similar things, but $tables is not made up of every $table, so it could be confusing.

I tend to name all query results as $result unless there is a potential naming conflict.

Concatenating a function result within a query call seems more a quality of 'fast code' than 'good code'.

Am I being too nit-picky? Does this code look like it was written by an amateur/bad programmer?

Answer 1

Your code looks like its good quality code. And yes, you’re right to be concerned. Most likely, after you’re gone, somebody else will (eventually) maintain, or change the code that you’ve written to date. I suggest that you comment and write the code so that a maintainer understands its purpose at first read.

For example ...

/*
 * Look quickly over the database to make sure nothing else is signed that ought to be unsigned.
 * TODO: Explain why unsigned is better than signed…
 */

$db = Database::instance();
$all_tables = array();
$result = $db->query('SHOW TABLES');
foreach($result as $unique_table)
{
   $show_created_table = $db->query("SHOW CREATE TABLE `".current($unique_table)."`");
   $all_tables[current($show_created_table->current())] = end($show_created_table->current());
}
echo var_dump($all_tables);

It's worth doing for yourself, for memory sake, if you haven't touched the code for a longtime. It will also help the maintainer understand the intented purpose of the function. Now, if your project is large and complex, I'd recommend that you use phpdoc -- it's a good standard.

Answer 2

Personally I find that a db query should return a query object, which should then have exposed result() and num_rows() etc methods. IMO don't like seeing things like $create->current(); - which I'm not actually sure I understand... What exactly are you achieving with the current method? Why is the db query method returning what seems like an array when first used, but then an object when used in the loop?