4
\$\begingroup\$

Is there any reason NOT to wrap a long query over multiple line via new lines vs using concatenation?

For example, consider this:

$this->query("SELECT u.*, COALESCE(c2.name,u.state) AS real_state, c.name AS real_country, bc.alpha3 AS country_iso, bs.code AS state_iso
                    FROM " . MAIN_DB . ".users AS u
                    INNER JOIN " . MAIN_DB . ".countries AS c ON u.country=c.id 
                    LEFT JOIN " . MAIN_DB . ".countries AS c2 ON u.state=c2.id 
                    LEFT JOIN " . BLESTA_DB . ".countries AS bc ON c.country_iso_code=bc.alpha2
                    LEFT JOIN " . BLESTA_DB . ".states AS bs ON bc.alpha2=bs.country_alpha2 AND COALESCE(c2.name,u.state)=bs.name
                    WHERE u.active=1 AND u.mail_activated=1 AND u.blesta_id=0");

VS

$this->query("SELECT u.*, COALESCE(c2.name,u.state) AS real_state, c.name AS real_country, bc.alpha3 AS country_iso, bs.code AS state_iso " . 
                    "FROM " . MAIN_DB . ".users AS u " . 
                    "INNER JOIN " . MAIN_DB . ".countries AS c ON u.country=c.id " . 
                    "LEFT JOIN " . MAIN_DB . ".countries AS c2 ON u.state=c2.id " . 
                    "LEFT JOIN " . BLESTA_DB . ".countries AS bc ON c.country_iso_code=bc.alpha2 " . 
                    "LEFT JOIN " . BLESTA_DB . ".states AS bs ON bc.alpha2=bs.country_alpha2 AND COALESCE(c2.name,u.state)=bs.name " . 
                    "WHERE u.active=1 AND u.mail_activated=1 AND u.blesta_id=0");

Are there any possible problems with the first code or is either fine?

\$\endgroup\$
0
2
\$\begingroup\$

COALESCE(c2.name,u.state) AS real_state seems bizarre. That would cause the user's country to always override the user's state. Perhaps you got the parameters reversed?

As for the code formatting, it's mostly a matter of opinion. There might be a tiny difference in performance, but given the other string concatenations that you are already doing, plus the fact that the execution of the query will completely dominate the running time anyway, it ends up being purely a matter of opinion.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you. As for your concern about the COALESCE, the code is correct. I inherited a system where countries AND states are all stored in the same table and only the more common countries have entries state entries in said table; so sometimes we have a connection in the countries table for the state (via a matching id in the users table) and hence the matched entry will be used, but if there is no match (user entered state) then it will skip that value as it will be NULL and it will use whatever state the user entered. :) \$\endgroup\$
    – Brett
    Sep 13 '14 at 12:47
5
\$\begingroup\$

The first way is less error-prone. When you write like this:

"FROM " . MAIN_DB . ".users AS u " . 
"INNER JOIN " . MAIN_DB . ".countries AS c ON u.country=c.id " .

... it's easy to "forget" to add a trailing space on the previous line, which is necessary to separate the clauses of the statement.

This might be slightly better, though it might look somewhat awkward:

" FROM " . MAIN_DB . ".users AS u " . 
" INNER JOIN " . MAIN_DB . ".countries AS c ON u.country=c.id " .

Here, it's easier to see the space before the expressions, as they are all aligned to the left, your eyes don't have to scan the right ends to see if you put a space there.

With your first version, this problem doesn't exist: the embedded newlines ensure that the clauses are separated, you cannot accidentally forget whitespace in between, it's naturally there. Another advantage of the first version is that it's easier to copy and paste to a query runner program.

Finally, when embedding variables in long strings, I find the interruptions caused by concatenation quite annoying, so I prefer to use formatting with a template, like this:

$query = sprintf(
    "SELECT u.*, COALESCE(c2.name,u.state) AS real_state, c.name AS real_country, bc.alpha3 AS country_iso, bs.code AS state_iso 
    FROM %s.users AS u 
    INNER JOIN %s.countries AS c ON u.country=c.id 
    LEFT JOIN %s.countries AS c2 ON u.state=c2.id 
    LEFT JOIN %s.countries AS bc ON c.country_iso_code=bc.alpha2 
    LEFT JOIN %s.states AS bs ON bc.alpha2=bs.country_alpha2 AND COALESCE(c2.name,u.state)=bs.name 
    WHERE u.active=1 AND u.mail_activated=1 AND u.blesta_id=0",
    MAIN_DB, MAIN_DB, MAIN_DB, BLESTA_DB, BLESTA_DB);

The drawback of this approach is that now the table names are not so easily visible in their contexts, which can invite mistakes in the argument list.

A helper function from this post on Stack Overflow could improve that:

function vsprintf_named($format, $args) {
    $names = preg_match_all('/%\((.*?)\)/', $format, $matches, PREG_SET_ORDER);

    $values = array();
    foreach($matches as $match) {
        $values[] = $args[$match[1]];
    }

    $format = preg_replace('/%\((.*?)\)/', '%', $format);
    return vsprintf($format, $values);
}

Like this:

$query = vsprintf_named(
    "SELECT u.*, COALESCE(c2.name,u.state) AS real_state, c.name AS real_country, bc.alpha3 AS country_iso, bs.code AS state_iso 
    FROM %(main).users AS u 
    INNER JOIN %(main).countries AS c ON u.country=c.id 
    LEFT JOIN %(main).countries AS c2 ON u.state=c2.id 
    LEFT JOIN %(blesta).countries AS bc ON c.country_iso_code=bc.alpha2 
    LEFT JOIN %(blesta).states AS bs ON bc.alpha2=bs.country_alpha2 AND COALESCE(c2.name,u.state)=bs.name 
    WHERE u.active=1 AND u.mail_activated=1 AND u.blesta_id=0",
    array('main' => MAIN_DB, 'blesta' => BLESTA_DB));
\$\endgroup\$
2
  • \$\begingroup\$ Some excellent points - thanks! I emphasize with your last point, but at the end of the day I prefer more readable code so I like to keep everything in place. :) \$\endgroup\$
    – Brett
    Sep 13 '14 at 13:59
  • \$\begingroup\$ I added one more alternative that's more readable, but it needs a helper function. \$\endgroup\$
    – janos
    Sep 13 '14 at 14:05

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.