"if" conditions spanning multiple lines are seldom, but happen. Example:

if ($include_unauthorized
    || versioncontrol_is_account_authorized($account->uid, $repositories[$account->repo_id])) {
  if (!isset($accounts[$account->uid])) {
    $accounts[$account->uid] = array();
  }
  $accounts[$account->uid][$account->repo_id] = $account->username;
}

(The first random one that I found in one of my projects - there are several more, some with even more text and spanning three lines.)

Now the Drupal coding guidelines tell us to put the opening brace on the same line, but for multiline if conditions that's just less readable. It would be much nicer if it looked like this:

if ($include_unauthorized
    || versioncontrol_is_account_authorized($account->uid, $repositories[$account->repo_id]))
{
  if (!isset($accounts[$account->uid])) {
    $accounts[$account->uid] = array();
  }
  $accounts[$account->uid][$account->repo_id] = $account->username;
}

because then you can actually tell apart the condition and the first line in the block, and in consequence have a good grasp of how much code the block spans, even if the condition itself is long and unwieldy.

So... can we please have the "{ needs to end a line, not start one" check disabled if the block-opener statement (i.e. "if") spans multiple lines? I'm always deciding between readability and Coder compliance on those occasions, and imho we should have both. Deal?

Comments

douggreen’s picture

douggreen commented

You'll need to start a coding standard discussion. And only if the coding standard is changed, will coder be changed. Personally, I think the current standard here is fine, and is very readable. But that's just my opinion. I'll leave this open to see if other's want to chime in, but I think that this is going to be a "won't fix".

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I can understand your motivation for this issue, Jakob, since I've run into similar cases in the past. Also, ezyang questioned exactly this coding standard during the GHOP code sprint while improving coder_format for D6.

Like always, there's no ultimate answer here. While

if (foo_bar_baz($foo, $bar, $baz)
  || baz_bar_foo($foo, $bar, $baz)
  || !$baz)
{
  return $foo;
}

might work out in certain situations, it would lead to a general assumption of

if ($foo
  || $bar
  || !$baz)
{
  return $foo;
}

or even

if ($foo)
{
  return $foo
}

which is definitely wrong according to Drupal's coding standards.

To circumvent situations like this, you can code around to fulfill this coding standard:

$account_authorized = versioncontrol_is_account_authorized($account->uid, $repositories[$account->repo_id]);
if ($include_unauthorized || $account_authorized) {
  if (!isset($accounts[$account->uid])) {
    $accounts[$account->uid] = array();
  }
  $accounts[$account->uid][$account->repo_id] = $account->username;
}

I know that this suggestion is not performance-orientied, but sometimes there are cases where coding standards actually have higher priority than performance.

Thus, I'd propose to mark this won't fix, too.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Component: Coder Format » Code
douggreen’s picture

douggreen commented
Status: Active » Closed (won't fix)

And until there's a coding standard change, coder won't allow this.