Relax the "Conditions should not be wrapped into multiple lines" rule for more code readability

Update to use the new coding standards issue template.

I got confused a few times because we have a standard regarding 80 characters per line, but it doesn't apply for "if" conditions. So if we accept option A, then we kill two birds with one stone: readability and minus the exception to the general rule. My vote for "multiple lines".

+1. As someone who programs custom modules like this, I always forget to not do this with core patches.

There are so many reasons to want this, inline with the same points on #3295249: Allow multi-line function declarations:

• improved code readability.
• cleaner, simpler diffs, patches, and git blame history.
• consistency with other non Drupal PHP projects.

Personally, I dislike like the examples in the IS where the first expression is inline with the opening parenthesis, and prefer the one where the first expression comes after a newline, and is indented by drupal's standard 2-space indent.

I suggest we propose the PSR12 format for multi-line if, elseif, else structures, (just like we did for #3295249) which would end up looking like this:

if (
  $expr1
  && $expr2
) {
  // if body
} elseif (
  $expr3
  && $expr4
) {
  // elseif body
}

IMO the code samples in issue summary should be updated accordingly for consistency.

I've added myself as a supporter. Thanks.

I've made a bit of cleanup to the grammar and code samples in the IS to make the proposal more concise and readable. It is still a bit long winded though.

I also added a more complete example on how to properly indent nested if conditions with AND/OR combinations.

if (
  $longExpr1
  && $longExpr2
  || (
     $longExpr3
     && $longExpr4
  )
) {

The example in Option A has therefore become perhaps a bit contrived, so it probably makes sense to work on an improved example there. Maybe we could find a realistic if statement from Drupal core that has a logical OR and a logical AND inside the conditions.

I support allowing multi-line conditions (didn’t even know they were explicitly discouraged), but the new rule should orient itself on what’s already in Core – i.e., do not require a line break in front of the first condition.

I also thought that subsequent lines should be double-indented (four spaces, not two) according to the Standards, but seems I was mistaken there. I’d think that’s a good idea, too, but definitely won’t vote for a change there, either.

Finally, mixing && and || in the same condition without clarifying parentheses seems terrible for readability. I would vote for explicitly discouraging/forbidding that (possible in a separate ticket, though), and am definitely against listing it as an example.

I support the idea of multi-line conditions, but I'm not sure I agree with this specific format:

  $expr1
  && $expr2

I would prefer a format where all of the expressions have the same indention, I think that might help with scanability. The way it is right now it kind of looks like the first expression has some kind of preferential treatment, or that the following expressions are not part of the same block.

I might suggest putting the logical operator at the end of the line, something like:

  $expr1 &&
  $expr2

But then I could see advantages to the logical operator being at the start of the line, so maybe something like:

     $expr1
  && $expr2

But then unless you have a mono-space font, it has the same problem where the expressions are not all vertically aligned (coincidentally the code-blocks do use a monospace font so not as easy to illustrate, but with a mono-space font I really like that). So I'm not sure that's any better.

Thoughts?

I'd prefer we use the thing already used in the php community.

One could argue that

  $expr1
  && $expr2
  && $expr3
  && $expr4
  && $expr5

makes more sense, since you first see your main condition (which should be the earliest false preferably) then the secondary, this gets most helpfull if there is more than 2, so i don't really think this is less readable.

I'd also opt for option a and describe in the documentation.

I personally put the operator on the end of the line, but if the PHP community is using it at the start, then that is fine with me. It makes sense to align ourselves.

This was discussed at the coding standards committee and it was pointed out that we need clarity around if there should be an initial condition on the first line with the if.

Can we get the options updated to add explicit guidance w.r.t that.

I agree that if the wider PHP community has adopted a standard, then it makes a lot of sense for us to follow that.

One could argue that [...] makes more sense, since you first see your main condition (which should be the earliest false preferably) then the secondary, this gets most helpfull if there is more than 2, so i don't really think this is less readable.

That's a very valid point!

On reflection, I am now more neutral and don't feel strongly in either direction.

Definitely a big supporter of this change. We should allow multi-line here.

+1 to a newline after the initial if ( before the first condition if we're going to split it. Makes it more in alignment with our other multi-line standards, and more in line with PSR12 and the rest of the community.

Also, I strongly believe that operators at the front of the line make for more readable and understandable code. When parsing boolean logic with your brain, it seems essential to know in advance if you're building up an OR vs. AND so you can keep that in mind while reading the rest of the line. I know it's somewhat subjective, but I think there's an objective point in here that the operator is essential for understanding the rest of what you're reading. 😅

Added myself as a supporter.

I just took some of the relevant text from the PSR-12 standard here, adapted it to our NO NEED TO SHOUT conventions, and added it to the proposed changes under option A. I hope that's a friendly amendment. I believe it's useful to be explicit that if you do this, you must format it according to clear rules, not just a block of example code.

Added myself as supporter.

Marked "List three supporters" task as done.

Moved to RTBC because concerns on #17 were addressed by #19 (first condition should be in next line and proposed text has been updated accordingly).

Lovely to see another issue at RTBC.

I have not read all of this issue. Honestly, I am stuck on understanding the proposed paragraph. For me it is dense and hard to follow. I made a first effort of using multiples lines so I could make sense of it. Then I changed some words.

• Conditions with a single expression should be on one line.
• Multiple conditions that cause the control structure to span more than the 80-character line length may be split across multiple lines.
• When multiple lines are used
  • each subsequent line is indented at least once.
  • The first condition must be on the next line.
• The closing parenthesis and opening brace must on a separate line and separated by a space.
• Boolean operators between conditions must be at the beginning or at the end of the line, not a mix of both.

And some questions.
"at least once" - Can someone explain what this means. It is meant to be one space? Can I indent is multiple spaces? How many?
Can we make a decision about boolean being at the beginning or the end? Why allow both?

What I also see as an possible improvement is to be carefull with the "negatives".

Conditions with a single expression should not be wrapped into multiple lines.
versus
Conditions with a single expression should be on one line.

The second setup seems a lot easier to read. Even if that means we need a little more text after taht.

"at least once" is indeed a little weird, and since we do have rules on what indentation should be perhaps something like the following is more clear:

Perhaps:
When multiple lines are used, the first condition starts on a new line, indented once. Each subsequent line is indented once for each level of nesting.

Regarding the booleans i'd probably say we should just always set them last (or not even mention them). Isn't this how all core code is right now? Seems weird to suddenly change that.

I compared the proposal to existing text on the same coding standards page and changed the proposal to the following. The first condition is now a positive statements and all points for multiple lines are in a sub group.

• Conditions with a single expression should be on a single line.
• Multiple conditions that exceed 80 characters may be split across multiple lines.
• When multiple conditions are split across multiple lines
  • Subsequent lines are indented.
  • The first condition must be on the next line.
  • The closing parenthesis and opening brace must be placed together on their own line with one space between them.
  • Boolean operators between conditions must always be at the beginning or at the end of the line, not a mix of both.

Also, I found that the sentence beginning "The closing" already exists on the page, in Function Declarations. It is probably better to use existing text.

#21 and #22 are still to do.

There is an older discussion in #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines where we also mention conditions with && and ||.

And for the record, I support:
- No line break before first condition.
- Operator && or || at the beginning of a line.
- Line breaks around the ") {".

if ($long_condition_a
  && $long_condition_b
  && ($long_condition_c1
    || $long_condition_c2
  )
) {
  return 'yes';

I do see the appeal of line break before the first condition, but somehow it feels wrong.. but this could be just a preference familiarity.

In the past I would have proposed something like below, but now I think it is awkward..

if (true
  && $long_condition_a
  && $long_condition_b
) {

I am strongly in favour of the "keep left" approach to operators (i.e. operators at the start of lines).

I was trying to find a reference for this recently for a colleague, and the best I could find online is in the Python PEP8 coding standards:

https://peps.python.org/pep-0008/#should-a-line-break-before-or-after-a-...

I think they lay it out pretty clearly.

When you read left-to-right and all the operators at a given level are aligned on the left, then your brain has to do very little work to see the structure of that code. Conversely, if you have to scan to the end of each line to figure out how it fits into the structure, you're working comparative overtime.

A colleague recommended this to me a very long time ago, and I've never been so immediately convinced by a formatting change in my life. I've done it this way ever since, and would gladly see it be part of the coding standards here. It's just better.

p.s. A few comments here are associating this approach with only "boolean operators" (I think meaning just && and ||), but this principle is not in any way constrained to those. "Keep left" is the reader-friendly position for any operator when breaking lines. The && and || operators are common of course, but they're arbitrary examples. It doesn't matter what the operator is, it's always better for the line to break before it rather than after (for exactly the same reasons and readability improvements that apply to the boolean cases).

FWIW, I would write the previous example as:

if ($long_condition_a
    && $long_condition_b
    && ($long_condition_c1
        || $long_condition_c2))
{
  return 'yes';
}

I find the newlines-after-( approach painfully verbose (and ugly), whereas I find the above lays out the structure in an extremely readable fashion, and with no wasted vertical space. In particular, the alignment at the parentheses makes it super easy to see the structure of the nesting.

Compare with the following version (one of the earlier suggestions) where, at a glance, the specific structure isn't quite so obvious, and so you have to work a little bit harder. There are now four three different columns in use rather than two, and components at the same nesting level are not aligned with one another.

if ($long_condition_a
  && $long_condition_b
  && ($long_condition_c1
    || $long_condition_c2
  )
) {
  return 'yes';
}

(I wrote "four three different columns" above because in fact || $long_condition_c2 is in line with $long_condition_a, as things are effectively being unindented; so it's only three, and we end up with different nesting levels that are visually overlapped.)

You can recover the better alignment by enforcing a newline after every (, but that then causes the really verbose waste of vertical space:

if (
  $long_condition_a
  && $long_condition_b
  && (
    $long_condition_c1
    || $long_condition_c2
  )
) {
  return 'yes';
}

That's now taking several additional lines yet doesn't (IMHO) achieve any benefit that isn't also present in my first, more compact example.

I think the waste of space is undesirable for two reasons. Firstly, obviously, you don't get to see as much of the surrounding code. Secondly, the coding standards should (as one of its goals) encourage readable code, and that includes encouraging authors to employ multi-line conditions whenever it can aid readability. But if the author's original inclinations are to put all of the conditions on a single line then they are already placing a high value on minimising vertical space, in which case the more verbose the alternative is, the more resistant they will be to using it. A more-readable-yet-relatively-compact approach wins on both counts.

(And of course this is only a small example. Make the sequence even moderately complex, and all the extra newlines will add up extremely quickly.)

If I had to choose between the 2nd and 3rd options then I'd vote for the 3rd because the benefit of conveying the code structure via alignment outweighs the detriment of the additional lines of code required. But I can't see any reason to pick the 3rd option over the 1st...

Regardless of all that, it's definitely important to have the { on a new line though (or the ) { if taking that approach), otherwise the body of the block isn't visibly indented from the conditions. So that's a must-have, to my mind.

Operators on the left is the one I'd fight for more than anything, though.

I’ve recently been working on projects using all three of the styles listed in #27, and am now of the opinion that the third style (newlines everywhere) is the most readable after all. The others just feel weirdly assymetric, and even after months working with them (especially the first) I still always stumble over them.

A quick scroll over usages in Core (git ls | grep '\.php$\|\.module$\|\.theme$' | xargs grep -A 4 '^ *if (.*[^{]$' | less) paints a rather chaotic picture (though there aren’t that many cases of this to begin with), so we’d have to change loads of them no matter which style we go with (if we want to make them consistent). Consequently, the fact that we currently don’t use the third style at all, as far as I can see, isn’t a particular problem in my opinion.

This needs an issue summary update to incorporate the discussion from #21 onward.