6
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Your style is very readable. Almost makes me feel like I would be able to pick up PHP with ease. \$\endgroup\$ – IAbstract May 12 '11 at 21:06
  • \$\begingroup\$ If you're concerned about possible confusion between two variable names, why not change one of them to eliminate the confusion? You've already identified a potential problem; whether it's a real problem or not is a matter of taste, but it won't take you 10 seconds to fix it. \$\endgroup\$ – Caleb May 13 '11 at 15:07
7
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

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?

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.