Enhance code readability

[Ryan R.]

Hello,

I'd like to propose a change to the PSR-2 rule concerning the presence of one (and only one) empty line after opening braces for conditional statements.

Indeed I believe allowing (enforcing ?) a single empty line at the beginning of the conditional statement would enhance the readability of the code, especially when multiple conditional statements are nested.

i.e : 

if ('string' === $dummy) { 

 
    if ('values' === $mergedOptions['some_default']) {

         return substr($dummy, 0, 5);
    }

    return ucwords($dummy);

}

instead of

if ('string' === $dummy) { 

    if ('values' === $mergedOptions['some_default']) { 
         return substr($dummy, 0, 5);
    }

    return ucwords($dummy);

}

[Alessandro Pellizzari]

On Sat, 4 Feb 2017 11:41:39 -0800 (PST)
"Ryan R." <ryan.r...@gmail.com> wrote:

> Hello,
> I'd like to propose a change to the PSR-2 rule concerning the
> presence of one (and only one) empty line after opening braces for
> conditional statements.
>
> Indeed I believe allowing (enforcing ?) a single empty line at the
> beginning of the conditional statement would enhance the readability
> of the code, especially when multiple conditional statements are
> nested.

I agree in allowing (not enforcing) blank lines in conditionals.

Personally I prefer a blank line before the }else{ part:

if ('string' === $dummy) {
    if ('values' === $mergedOptions['some_default']) {
        return substr($dummy, 0, 5);
   

} else {
$dummy = trim($dummy);
}

    return ucwords($dummy);
}

In this case it is probably not so evident, but I find it more readable
when there is a block of 5-6 lines.

I don't think a PSR-2 change is possible, but maybe it could be
included in PSR-12?

Bye.

[Woody Gilk]

I'll chime in and say that neither of proposed changes appeal to me.

--
You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/20170204230621.5fa888f5%40namek.
For more options, visit https://groups.google.com/d/optout.

--

Woody Gilk

http://about.me/shadowhand

[Niels Braczek]

Am 05.02.2017 um 00:06 schrieb Alessandro Pellizzari:

> On Sat, 4 Feb 2017 11:41:39 -0800 (PST) Ryan R. wrote:
>
>> Indeed I believe allowing (enforcing ?) a single empty line at the
>> beginning of the conditional statement would enhance the readability
>> of the code, especially when multiple conditional statements are
>> nested.
>
> I agree in allowing (not enforcing) blank lines in conditionals.
>
> Personally I prefer a blank line before the }else{ part:

The readability is IMHO enhanced best by placing the braces on their own
lines:

if ('string' === $dummy)
{
if ('values' === $mergedOptions['some_default'])
{
return substr($dummy, 0, 5);
}
else
{
$dummy = trim($dummy);
}

return ucwords($dummy);
}

With that rule, you get the optical block separation out of the box.
Start and end of a block is easily recognized, because the corresponding
braces are always on the same column.

> I don't think a PSR-2 change is possible, but maybe it could be
> included in PSR-12?

Agree.

Regards,
Niels

--
| New Stars on the Horizon: GreenCape · nibralab · laJoom |
| http://www.bsds.de · BSDS Braczek Software- und DatenSysteme |
| Webdesign · Webhosting · e-Commerce · Joomla! Content Management |
------------------------------------------------------------------

[Korvin Szanto]

Hi Everyone,

On Sat, Feb 4, 2017 at 11:41 AM Ryan R. <ryan.r...@gmail.com> wrote:

Hello,

I'd like to propose a change to the PSR-2 rule concerning the presence of one (and only one) empty line after opening braces for conditional statements.

I wasn't aware that this was the case, can you link to where in PSR-2 you're seeing that? From what I understand PSR-2 currently allows blank lines and even (sort of) encourages adding them to improve readability [1]:

> Blank lines MAY be added to improve readability and to indicate related blocks of code.

 so we should be all set on this front.

[1] http://www.php-fig.org/psr/psr-2/#lines

Best wishes,

Korvin

[Ryan R.]

can you link to where in PSR-2 you're seeing that?

http://www.php-fig.org/psr/psr-2/#if-elseif-else >> see the "An if structure looks like the following" sentence and following code sample

I personnally did not pay attention to that sentence the first time around and only looked at the rules noted at http://www.php-fig.org/psr/psr-2/#control-structures

But after getting a phpcs error on this précise type of case, I created an issue in their github (https://github.com/squizlabs/PHP_CodeSniffer/issues/1330)

I was told to pay attention at that precise line on the PSR.

IMHO, thsi is not clear as currently stated in the PSR :

I strongly believe allowing for one blank line at the beginning of controle structure is useful / necessary.

I also agree that Iitcould be useful to allow one at the end of the control structure parts (see Alessandro's comment):

Personally I prefer a blank line before the }else{

However I'm puzzled with Nielz proposition, not on the fact that it would increase readability, but more on the fact that it would mean reviewing the entire concept of the braces placements in the PSRs. 

Even though if I myself am quite annoyed by the decision taken to have two different type of placements... I'd rather have only one (either on next line or same line with an empty line allowed after)

[Ryan R.]

P.S. : The sentence about blank lines should also be modified in PSR-12 proposal as, following the logic of the "code should look like this sample" rule the following would go against any decision made in relation to allowing blank lines in control structures alltogether :

"Blank lines MAY be added to improve readability and to indicate related blocks of code except where explictly forbidden."

[Greg Sherwood]

Note that if the decision is that PSR-2 does not enforce no blank lines at the start of control structures, it also does not enforce 0 or 1 blank lines, and so this would be valid code under PSR-2:

if ($foo) {

    echo $foo;

} else {

    echo $bar;

}

I use an extreme example here to show how relaxing coding standards can lead to unexpected results and thus inconsistency. It seems to me that developers relying on PHPCS to keep their code clean through checking and auto-fixing will now have to use custom rulesets to enforce what is a pretty standard rule. That feels like a big step backwards.

Right now, developers who do not want this rule enforced in any way can use a custom ruleset and exclude it pretty easily. Developers who want to enforce a single blank line at the start would need to write a custom sniff because none of the included coding standards have that rule. But that custom sniff could be contributed to the PHPCS project so it is available for everyone to use more easily.

Greg

[Greg Sherwood]

There is an extra bit of information I wanted to share about this.

I use PHPCS to analyse a number of PHP projects and determine what the common coding standards are in the PHP community: https://squizlabs.github.io/PHP_CodeSniffer/analysis/

I add a new metric today to check the number of blank lines at the top and bottom of control structures just to see how much variation there is. I don't have all the historical data, but the results for blank lines at the start of control structures are here: https://squizlabs.github.io/PHP_CodeSniffer/analysis/#blank-lines-at-start-of-control-structure

Summary:

Checked 537,792 control structures in 193 projects

All 193 projects prefer no blank lines at the top

The lowest percentage was 77% of control structures with no blank lines, but most are 90-100%

1.02% of all control structures used 1 blank line at the top

98.98% of all control structures used 0 blank lines at the top

This obviously isn't data covering all PHP projects, but it covers most of the popular ones that are using PSR-2 and that are accepting contributions from people. These projects obviously have a rule that no blank lines should be at the top of control structures and are expecting that their coding standard enforces it.

I don't think it would be wise to change the way PHP_CodeSniffer enforces blank lines at the top of control structures without a piece of PSR-2 errata making it official.

Greg

[Ryan R.]

I use an extreme example here to show how relaxing coding standards can lead to unexpected results and thus inconsistency. It seems to me that developers relying on PHPCS to keep their code clean through checking and auto-fixing will now have to use custom rulesets to enforce what is a pretty standard rule. That feels like a big step backwards.

 

@Greg : Would it not be possible to add some kind of parameter value to indicate the upper limits authorized ? And set it to 1 by default (or 0 since you do not want to modify the current system because of the number of projects relying on it) ?

Then if we want to allow empty lines we could simply modify the parameter value !

[Greg Sherwood]

On Monday, 6 February 2017 20:45:47 UTC+11, Ryan R. wrote:

I use an extreme example here to show how relaxing coding standards can lead to unexpected results and thus inconsistency. It seems to me that developers relying on PHPCS to keep their code clean through checking and auto-fixing will now have to use custom rulesets to enforce what is a pretty standard rule. That feels like a big step backwards.

 

@Greg : Would it not be possible to add some kind of parameter value to indicate the upper limits authorized ? And set it to 1 by default (or 0 since you do not want to modify the current system because of the number of projects relying on it) ?

Then if we want to allow empty lines we could simply modify the parameter value !

 

Yes, that's what I was trying to suggest in my reply. Either way, some set of developers would not be able to use the included PSR2 standard as-is - they would need to use a ruleset.xml file and either include a specific sniff or change a sniff option.

Given that developers who want to allow a single blank line already have to use a custom ruleset, keeping the default at 0 blank lines means those developers can change their ruleset from excluding the sniff to changing one of its settings - making it more useful.

But obviously, the side effect is that you're not following PSR2 as PHPCS sees it. I don't think this makes any difference (any consistency is good) but maybe others do.

If you think an option would solve your specific problem and you're not too concerned about if PSR-2 in PHPCS enforces 0 or 1 blank lines, then I'd suggest opening a new Github issue and we can work out a solution along those lines.

Greg

[Korvin Szanto]

I agree that this could be organized to be easier to understand at a glance, but I don't understand why this is still an issue. It sounds like PHPCS is assuming that code needs to exactly match examples in every way. That's simply not the case. I've shown that the document says specifically that blank lines are allowed for readability, can anyone show anything in the document that says differently?

What I would do is take this to PHPCS. I don't think this deserves errata, instead we should take care with PSR-12 to make sure that this misunderstanding doesn't continue.

Best wishes,
Korvin

--

You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/10a99a91-547e-4cc9-b05f-d58f2b6ef7b4%40googlegroups.com.

[Alexander Makarov]

Changes could not be done to PSR-2. There's now PSR-12 in the making.

As for the proposal itself, I find it unnecessary to force it. My personal preference is not to have empty lines in this case.