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".

I've created a change record about this: https://www.drupal.org/node/3398029 →

Added Maxim Motuzko as a supporter, one more supporter still wanted!

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.

@jwilson3, thank you, I also prefer a way where the first expression comes after a newline, and pointing to the PSR12 is a good point too!

So, I updated the issue summary. If you agree with it, can I add you to the list of supporters?

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 statements.

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.

Thanks! For me now all looks good, so let's mark it as "Reviewed & tested by the community" then.

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.

"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.

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, without the waste of vertical space.

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.