[Poll] Zend Coding Standard 2: Aligning statements, arrays and comment tags

xtreamwayz · September 11, 2018, 2:59pm

If you haven’t seen it yet, work is being done on Zend Coding Standard 2. Most rules we have so far are based on the coding style used in the newer zend-expressive* packages. However there is a big question left and we can’t agree on it:

To align or not to align?

Should we align equal statements, arrays and phpdoc comment tags or not? There is no right or wrong here, it’s personal preference, but we do like to know your opinion. Examples are below the poll.

Yes, align equal statements, arrays and phpdoc comment tags.
No, do not align anything.

0 voters

Arguments I’ve seen so far:

The readability benefits are typically not huge, and enforcing it means a lot of extra changes for what are often trivial additions.

Aligning brings back extra line changes in commits and that’s exactly what the trailing comma prevents.

Readability is king, and git diff , git blame/annotate as well as the github and gitlab UI have flags for ignoring whitespace changes and so do modern IDEs like PhpStorm.

Examples:

Statement not aligned:

$shortVar = (1 + 2);
$veryLongVarName = 'string';

Statement aligned:

$shortVar        = (1 + 2);
$veryLongVarName = 'string';

Array not aligned:

$serializedRequest = [
    'method' => 'POST',
    'request_target' => '/foo/bar?baz=bat',
    'uri' => 'http://example.com/foo/bar?baz=bat',
    'protocol_version' => '1.1',
    'headers' => [
        'Host' => ['example.com'],
        'Accept' => ['application/json'],
        'X-Foo-Bar' => [
            'Baz',
            'Bat',
        ],
    ],
    'body' => '{"test":"value"}',
];

Array aligned:

$serializedRequest = [
    'method'           => 'POST',
    'request_target'   => '/foo/bar?baz=bat',
    'uri'              => 'http://example.com/foo/bar?baz=bat',
    'protocol_version' => '1.1',
    'headers'          => [
        'Host'      => ['example.com'],
        'Accept'    => ['application/json'],
        'X-Foo-Bar' => [
            'Baz',
            'Bat',
        ],
    ],
    'body'             => '{"test":"value"}',
];

Function comment not aligned:

/**
 * @param array $serverParams Server parameters, typically from $_SERVER
 * @param array $uploadedFiles Upload file information, a tree of UploadedFiles
 * @param null|string|UriInterface $uri URI for the request, if any.
 * @param null|string $method HTTP method for the request, if any.
 * @param string|resource|StreamInterface $body Message body, if any.
 * @param array $headers Headers for the message, if any.
 * @param array $cookies Cookies for the message, if any.
 * @param array $queryParams Query params for the message, if any.
 * @param null|array|object $parsedBody The deserialized body parameters, if any.
 * @param string $protocol HTTP protocol version.
 * @throws Exception\InvalidArgumentException for any invalid value.
 */

Function comment aligned:

/**
 * @param array                           $serverParams  Server parameters, typically from $_SERVER
 * @param array                           $uploadedFiles Upload file information, a tree of UploadedFiles
 * @param null|string|UriInterface        $uri           URI for the request, if any.
 * @param null|string                     $method        HTTP method for the request, if any.
 * @param string|resource|StreamInterface $body          Message body, if any.
 * @param array                           $headers       Headers for the message, if any.
 * @param array                           $cookies       Cookies for the message, if any.
 * @param array                           $queryParams   Query params for the message, if any.
 * @param null|array|object               $parsedBody    The deserialized body parameters, if any.
 * @param string                          $protocol      HTTP protocol version.
 * @throws Exception\InvalidArgumentException for any invalid value.
 */

If you have additional thoughts or comments, please let us know.

Tigerman55 · September 11, 2018, 3:56pm

I used to align but changed because some long variable names and array configs can make the even alignment hard to read. It is easier to read without alignment in my opinion.

ocramius · September 11, 2018, 4:24pm

As somebody that reads a dew thousands LOC a day, alignment is welcome and useful: code that is not aligned in logical blocks takes a lot of time to read and follow.

Logical assignment grouping is also unclear when code is nor aligned, since a non-assignment operation may sneak in the block of non-aligned statements, making things even harder to scan.

Overall, given the amount of time that I spend reading vs writing code, any improvement that makes it easier to group together declarations and operations is welcome.

thexpand · September 11, 2018, 8:39pm

My eyes bleed when I see non-aligned code. Having the ability to visually group similar variable assignments and array properties is more important than not having extra lines in the commit.

As you can tell from the example with the aligned array above, there is a slight drawback, related to multi-level nested arrays. Sometimes the deepest level could have less indentation than a higher level element. However, such arrays are not so likely to exist, as arrays are mostly used in module configuration, which now lives inside the almost flat ConfigProvider. What I’m trying to say is that we can disregard this downside and take advantage of the other stuff the alignment provides.

That’s why I strongly vote to align!

tux-rampage · September 12, 2018, 5:56pm

When it comes to writing code I’ve better things to do than aligning all code. IMHO it’s a waste of time for a very small benefit. The key question for me is: are there tools that can do this automatically? Can this be automated with cs-fix via the coding-standard-packge? If not then I’d argue that we’re wasting valuable time of the contributors. And honestly I don’t find the aligned Arrays more readable. Aligning phpdoc and variable assignments may improve readability a bit. My vote for no, as long as this can’t be automated.

thexpand · September 12, 2018, 6:25pm

@tux-rampage
PhpStorm provides this in their formatting tool (code style).
PHP-CS has it already - you can check it here: https://github.com/FriendsOfPHP/PHP-CS-Fixer
(search for binary_operator_spaces)

ocramius · September 12, 2018, 6:49pm

And also, the linked patch (from the OP) already does the alignment part.

xtreamwayz · September 12, 2018, 7:40pm

Aligning statements, docblocks and arrays can be done automatically with phpcbf or composer cs-fix in ZF components. However reverting aligned docblocks and arrays cannot be done automatically AFAIK.

The whole reason for that PR is to automate checking and fixing code. Only some rules are not automated (yet).

tux-rampage · September 12, 2018, 8:06pm

Good to know, then forget about my concerns. Since this can be automated, I don’t see a reason why not.

froschdesign · September 17, 2018, 6:33am

If we can not automatically reverting the alignment, how should this be fixed in all existing components if the poll result is “no”?

xtreamwayz · September 17, 2018, 8:00am

Let’s see what the result is first. After that we’ll find a way to enforce it. I don’t think we should decide for a code style because the way we actually wanted has no existing rule.

froschdesign · September 17, 2018, 8:18am

My question is based on your statements:

However reverting aligned docblocks and arrays cannot be done automatically AFAIK.
…
The whole reason for that PR is to automate checking and fixing code.

Right, we want automatism and this with a minimum of extra code and custom rules. But you say this cannot be done automatically?!

After that we’ll find a way to enforce it.

So we can only hope that it is easy.

xtreamwayz · September 17, 2018, 3:23pm

I don’t think so but I’m not 100% sure. It takes a lot of time tweaking rules like this and I stopped specifically for this rule until the voting is closed.

I’m pretty sure that you can call 0 new classes to enforce about 120 rules “a minimum of extra code and custom rules”

osh · September 19, 2018, 1:57pm

I was just wondering how would an operation with assignment work with that? For instance:

<?php
$int    = 1;
$string = 'foo';
$string .= 'bar';
// or
$int     = 1;
$string  = 'foo';
$string .= 'bar';

xtreamwayz · September 20, 2018, 2:25pm

That would be the second one (taken from the tests):

$string  = $foo . $bar . 'baz' . 'quux' . $string;
$string .= $foo;
$string .= $foo;
$string .= $foo;

thexpand · September 20, 2018, 2:32pm

I asked this in Slack, but I think its place is here, actually.

Is the alignment rule valid for class constants, too?

public const ID  = 'id';
public const SKU = 'sku';

or

public const ID = 'id';
public const SKU = 'sku';

xtreamwayz · September 20, 2018, 2:39pm

Currently the first one is enforced.

xtreamwayz · September 20, 2018, 2:39pm

Thank you all for voting.

Right now webimpress is working on an improved sniff for arrays. Once that is done we can test the rules against different components and discuss edge cases and possible adjustments.

thexpand · September 20, 2018, 2:47pm

I don’t know if it’s a problem, but I’ve tried to find a code style rule in PhpStorm for the alignment that @osh has given example for. When formatting with PhpStorm, the result is the non-aligned version of the string concatenation, unfortunately.

Topic		Replies	Views
Interface suffix: take it or leave it? Contributors	18	4533	September 8, 2017
Zend Db Sql Comments Components & MVC zend-db	0	747	February 2, 2018
Greeting for joing	0	694	May 6, 2017
Resources for ZF 1.12 project? Migration	3	671	March 4, 2021
Book: "Zend Framework 3. Developer's guide" Show and Tell	4	2473	August 31, 2023

[Poll] Zend Coding Standard 2: Aligning statements, arrays and comment tags

Related Topics