Coding style proposal: double quotes for SQL

Coding style proposal: double quotes for SQL

door David Mudrák -
Aantal antwoorden: 14
Foto van Core developers Foto van Documentation writers Foto van Moodle HQ Foto van Particularly helpful Moodlers Foto van Peer reviewers Foto van Plugin developers Foto van Plugins guardians Foto van Testers Foto van Translators
This is a proposal for adding the following rule into the Development:Coding_style.

Use double quotes for SQL statements, database table names and database field names. This does not influence functionality but with a syntax highlighting properly set, this makes the code more readable.

See the attached screenshot for an example of such highlighting.

Bijlage sql-highlight.png
Gemiddelde van de beoordelingen:  -
Als antwoord op David Mudrák

Re: Coding style proposal: double quotes for SQL

door Ravishankar Somasundaram -
David ++


I too have felt the same , and have been coding till now in this manner,
it also makes things easier for me, instead of starting a single quote and then double quote inside for making interpolation of variables,i just use a double quote at the start and then end it with the same,many consider that as a bad practice but no body is justifying why.

This will work for single level variable transformations ex:

echo " some text $textdesc , end of text ";


Gemiddelde van de beoordelingen:  -
Als antwoord op David Mudrák

Re: Coding style proposal: double quotes for SQL

door Aaron Barnes -
Hi David,

Moodle 2 is making the switch to prepared statements - using placeholders for data in SQL statements.

This means you will no longer have to frequently append values and double quotes would be superfluous.

Here is some more info about the new database layer:
http://docs.moodle.org/en/Development:DB_layer_2.0

Cheers,
Aaron
Gemiddelde van de beoordelingen:  -
Als antwoord op Aaron Barnes

Re: Coding style proposal: double quotes for SQL

door David Mudrák -
Foto van Core developers Foto van Documentation writers Foto van Moodle HQ Foto van Particularly helpful Moodlers Foto van Peer reviewers Foto van Plugin developers Foto van Plugins guardians Foto van Testers Foto van Translators

Aaron, thanks for the comment.

Yes, I know the new DB layer and I know how the SQL code looks like in 2.0. Also, using variables inside SQL statement has nothing to do with prepared statements but with binding parameters (syntax is the same but the concept is different). Still, there are situations where you use single quotes (') inside statements - to quote static strings, for example. And, there is a quite often case for putting variables into statements, typically the result of $DB->get_in_or_equal() call.

But apparently I have not underlined the key point here. My proposal has nothing to do with variables expansion. The main reason why I proposed is:

  • Petr already uses this syntax for all SQL's he has touched in 2.0. So this is not new at all.
  • syntax highlighting - even this may appear unimportant, it really counts if you need to quickly find something.
This means that double quotes should be also used at places like $DB->get_records() parameters - again for quick spotting of any SQL stuff.


Gemiddelde van de beoordelingen:  -
Als antwoord op David Mudrák

Re: Coding style proposal: double quotes for SQL

door Eloy Lafuente (stronk7) -
Foto van Core developers Foto van Documentation writers Foto van Moodle HQ Foto van Peer reviewers Foto van Plugin developers Foto van Testers
Hi David,

does the double/single quote make any difference really from a syntax highlight perspective?

I think I haven't anything special in my vim configuration causing both quoted strings to be displayed exactly the same.

Just note I'm using php_sql_query = 1 to highlight the SQL keywords differently within those strings but I get both perfectly pink (brown in your example).

In any case, I really think it's a matter of each IDE capabilities how those strings are displayed and that must not be the cause for any change in our coding style.

And, talking about coding... it used to be generally accepted to use single quotes when possible, afaik. It had some (minor I guess) parsing benefits and so on.

Ciao glimlach
Bijlage quotesandquotes.png
Gemiddelde van de beoordelingen:  -
Als antwoord op Eloy Lafuente (stronk7)

Re: Coding style proposal: double quotes for SQL

door David Mudrák -
Foto van Core developers Foto van Documentation writers Foto van Moodle HQ Foto van Particularly helpful Moodlers Foto van Peer reviewers Foto van Plugin developers Foto van Plugins guardians Foto van Testers Foto van Translators
The proposed solution is suitable for all IDEs that are able to distinguish between single quoted strings and double quoted strings. Anyway, this was just an idea. No press. In fact, this was originally Petr's idea who "forced" me to use this syntax during his review of the Workshop code...
Gemiddelde van de beoordelingen:  -
Als antwoord op David Mudrák

Re: Coding style proposal: double quotes for SQL

door Dan Poltawski -
I really don't like this proposal, for three reasons:

1) I have slight obsessive compulsive disorder when seeing string literals in double quotes. The age old wisdom that, its faster and more memory efficient as php doesn't need to do complicated string parsing makes me feel like i'm boosting performance with every double quoted string converted to single quotes!

2) I don't think we should be modifying the coding guidelines to fix broken syntax highlighting! We surely should be patching the syntax highlighter to do the right thing!

3) In non-syntax highlighted world (or if you don't trust your syntax-highlighter) I think there are readability benefits to distinguishing string literals being properly distinguished.

For example: I'm doing a security audit for SQL injection attacks I have to look a lot more closely at double quoted string to examine the contents of the string deeply to check for variables which might sneak in. With single quoted I don't have to look so deeply..
Gemiddelde van de beoordelingen:  -
Als antwoord op Dan Poltawski

Re: Coding style proposal: double quotes for SQL

door Tim Hunt -
Foto van Core developers Foto van Documentation writers Foto van Particularly helpful Moodlers Foto van Peer reviewers Foto van Plugin developers
I agree with Dan. -1 for David's original proposal.
Gemiddelde van de beoordelingen:  -
Als antwoord op Tim Hunt

Re: Coding style proposal: double quotes for SQL

door Petr Skoda -
Foto van Core developers Foto van Documentation writers Foto van Peer reviewers Foto van Plugin developers
I do not agree with Dan, +1 from me.

Majority of SQL code in 1.9 and HEAD is already in "double quotes". Making it consistent makes sense to me. Converting everything to ' does not seem to be an option here.

Petr
Gemiddelde van de beoordelingen:  -
Als antwoord op Petr Skoda

Re: Coding style proposal: double quotes for SQL

door Mark Johnson -
Foto van Core developers Foto van Particularly helpful Moodlers Foto van Peer reviewers Foto van Plugin developers
Surely making it consistent should be the other way around? All non-SQL strings (and some SQL strings) use single quotes. In the context of the PHP what the string contains doesn't matter, so just because they contian SQL doesn't mean they should be represented differently.
I don't think the coding guidelines should be changed to allow for where they haven't been followed - the code should be brought into line.

Additionally, I don't think that syntax highlighting is a good enough reason for introducing a coding convention, since it's not universal. If you follow the convention of putting SQL keywords in uppercase, then SQL strings stand out amongst all other code, which according to the guidelines should be lowercase.

Gemiddelde van de beoordelingen:  -
Als antwoord op Mark Johnson

Re: Coding style proposal: double quotes for SQL

door Mark Johnson -
Foto van Core developers Foto van Particularly helpful Moodlers Foto van Peer reviewers Foto van Plugin developers
An additional point - since {braces} in a double-quoted string are used for interpolation (is that the right word?), they could be a cause for confusion, as the new database API uses them to enclose table names. For example:

$sql = 'SELECT * FROM {user} WHERE id = 3;';
$sql = "SELECT * FROM {user} WHERE id = 3;";
Would behave the same, but
$sql = 'SELECT * FROM {$table->name} WHERE id = 3;';
$sql = "SELECT * FROM {$table->name} WHERE id = 3;";
wouldn't.

If I saw a double-quoted string containing something in braces I'd immediately assume it was being used for interpolation, and since all SQL strings are to contain them from now on where they AREN'T being used for interpolation I think double-quotes are a bad idea.
Gemiddelde van de beoordelingen: Useful (1)
Als antwoord op Petr Skoda

Re: Coding style proposal: double quotes for SQL

door Penny Leach -
I agree with Dan and Tim.

I hate double quotes glimlach And Mark's point about table placeholders {} makes a lot of sense as a reason to use single quotes as well.

(PS eloy thanks for the vim-tip ;) )
Gemiddelde van de beoordelingen:  -
Als antwoord op Penny Leach

Re: Coding style proposal: double quotes for SQL

door Martín Langhoff -
My much-diluted vote comes in for Eloy, Dan, Tim, Penny and Mark.

Specially with the new SQL table syntax, and the use of placeholders, there is little reason to use doublequotes.

If we're to consolidate our usage of quotes in SQL, it'd make sense to consolidate on single quotes. This will make odd code immediately stand out.
Gemiddelde van de beoordelingen:  -
Als antwoord op David Mudrák

Re: Coding style proposal: double quotes for SQL

door Petr Skoda -
Foto van Core developers Foto van Documentation writers Foto van Peer reviewers Foto van Plugin developers
Great, we have a winner - the single quotes grote grijns

I think we could wait a bit for the final word from MD and then convert all SQL to this new coding style, any objections? I hope not, patches can not merge due to previous changes anyway.

Petr
Gemiddelde van de beoordelingen:  -
Als antwoord op Petr Skoda

Re: Coding style proposal: double quotes for SQL

door Martin Dougiamas -
Foto van Core developers Foto van Documentation writers Foto van Moodle HQ Foto van Particularly helpful Moodlers Foto van Plugin developers Foto van Testers
Shrug, I really don't think this is too important to standardise, even though I'd probbaly put single quotes in the coding guidelines for new code.

I'd leave old stuff as it is and let it get slowly cleaned up when people edit it.
Gemiddelde van de beoordelingen:  -