I was just wondering if we have, or if we should have, any SQL coding standards regarding whitespace usage. For example, should we be using a space between column names in IN where clauses or have spaces on either side of comparison operators, etc.
Here are some examples to illustrate what I mean:
SELECT a,b,c FROM {table} WHERE a IN (1,2,3) AND b=4;
vs
SELECT a, b, c FROM {table} WHERE a IN (1, 2, 3) AND b = 4;and
INSERT INTO {table} (a,b,c) VALUES(1,2,3);
vs
INSERT INTO {table} (a, b, c) VALUES (1, 2, 3);I've checked the SQL coding conventions doc and searched d.o but didn't find anything on this. Core, itself, doesn't seem to follow a standard on this and I found a mixture of the above used. However, the majority of the existing core queries seem to advocate the use of spaces.
Personally I find the addition of the spaces makes the SQL query more readable. Maybe it doesn't make much of impact in these simple examples, but for more complex queries, I could really see this having a benefit for readability.
Thoughts?
Comments
As far as I know, we have no
As far as I know, we have no specified standard.
I think the unspoken agreement, however, is to "be like PHP" - i.e., be like our existing PHP coding standards, which would mean spaces after lists (array or otherwise) and spaces around operands.
Basically, yes. However...,
Basically, yes.
However..., I think DBTNG makes this question obsolete, since all queries should use the new methods instead.
It might make sense to discuss coding standards for DBTNG stuff instead.
Looking at node.module, this looks acceptable:
<?phpdb_update('node_type')->fields($fields)->condition('type', $existing_type)->execute();
...
$fields['orig_type'] = (string) $type->orig_type;
db_insert('node_type')->fields($fields)->execute();
...
$query = db_delete('node_access')->condition('nid', $node->nid);
if ($realm) {
$query->condition('realm', array($realm, 'all'), 'IN');
}
$query->execute();
?>
Looking at actions.inc, I like:
<?php$query = db_select('actions');
$query->addField('actions', 'aid');
$query->addField('actions', 'type');
$query->addField('actions', 'callback');
$query->addField('actions', 'parameters');
$query->condition('aid', $conditions, 'IN');
$result = $query->execute();
?>
However, still looking at actions.inc, the following looks totally disgusting:
<?php
db_insert('actions')
->fields(array(
'aid' => $callback,
'type' => $array['type'],
'callback' => $callback,
'parameters' => '',
'description' => $array['description'],
))
->execute();
...
$results = db_select('actions')
->addField('actions', 'aid')
->addField('actions', 'description')
->condition('callback', $orphaned, 'IN')
->execute();
...
if (!$aid) {
$aid = db_insert('actions_aid')->execute();
}
db_merge('actions')
->key(array('aid' => $aid))
->fields(array(
'callback' => $function,
'type' => $type,
'parameters' => serialize($params),
'description' => $desc,
))
->execute();
?>
Primarily, because the chained
->foo()method calls visually do not look clean and it's rather hard to track the indentation. Also, those result in total inconsistency of how we write queries.Daniel F. Kudwien
unleashed mind
Daniel F. Kudwien
netzstrategen
However..., I think DBTNG
Not all. Static SELECT queries still use the traditional methods. See http://drupal.org/node/310072
Consistency
I think the problem I have with the proposed DBTNG style is that the coder_format script was able to reformat any PHP code to Drupal's style until now. If we introduce a "fluent" DBTNG style like this, coder_format most probably is no longer able to take the same, complex decisions as we (humans) do. A clear and clean style is needed, which is comprehensible for all developers (without looking it up in a handbook first).
For example:
<?phpdb_insert('actions')
->fields(array(
'aid' => $callback,
'type' => $array['type'],
'callback' => $callback,
'parameters' => '',
'description' => $array['description'],
))
->execute();
?>
Based on what do we decide that ->fields() and ->execute() go on a new line? When do we write the passed-in array in one line?
Is the indentation of the two closing braces right? No, it is not. One has to count the braces first:
<?phpdb_insert('actions')
->fields(array(
'aid' => $callback,
'type' => $array['type'],
'callback' => $callback,
'parameters' => '',
'description' => $array['description'],
))
->execute();
?>
This is approximately, how the output of coder_format would look like:
<?phpdb_insert('actions')->fields(array(
'aid' => $callback,
'type' => $array['type'],
'callback' => $callback,
'parameters' => '',
'description' => $array['description'],
))->execute();
?>
I.e. method calls are appended, and multi-line arrays span over multiple lines like everywhere else.
Of course, in this case:
<?php$results = db_select('actions')
->addField('actions', 'aid')
->addField('actions', 'description')
->condition('callback', $orphaned, 'IN')
->execute();
?>
it wouldn't work out, since the line would exceed 80 chars (Also, do we really want to enforce this for queries? We did not do so in the past.):
<?php$results = db_select('actions')->addField('actions', 'aid')->addField('actions', 'description')->condition('callback', $orphaned, 'IN')->execute();
?>
But then again, when do we use the following syntax - and why don't we use it in such cases?
<?php$query = db_select('actions');
$query->addField('actions', 'aid');
$query->addField('actions', 'type');
$query->addField('actions', 'callback');
$query->addField('actions', 'parameters');
$query->condition('aid', $conditions, 'IN');
$result = $query->execute();
?>
Daniel F. Kudwien
unleashed mind
Daniel F. Kudwien
netzstrategen
db_select() not fluid
The last example of db_select() is the only correct one, it doesn't use the fluid chaining like db_insert() and db_delete() - I think primarily so that views can utilise it later on. So the chaining is only an issue for insert/update/delete.
Well, honestly, I think they
Well, honestly, I think they all look horrific compared to clean SQL, but eh ;)