HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Working with databases in PHP

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withphpworkingdatabases

Problem

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?

Solution

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.

Code Snippets

/*
 * 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);

Context

StackExchange Code Review Q#2370, answer score: 7

Revisions (0)

No revisions yet.