New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coding style : use multiline class and method signatures #18890

Closed
greg0ire opened this Issue May 26, 2016 · 23 comments

Comments

Projects
None yet
5 participants
@greg0ire
Contributor

greg0ire commented May 26, 2016

Not sure if this is a standard or what, but it looks like multiline signatures are absolutely not used in the codebase of Symfony. See for instance this 500 characters wide signatures : https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Http/Firewall/SimpleFormAuthenticationListener.php#L58

I'm ready to make a PR if you want to set a limit.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz May 26, 2016

Member

As you know Symfony follows the PSR-2. In that standard we can read:

Argument lists MAY be split across multiple lines, where each subsequent line is
indented once. When doing so, the first item in the list MUST be on the next line,
and there MUST be only one argument per line.

When the argument list is split across multiple lines, the closing parenthesis and
opening brace MUST be placed together on their own line with one space between them.

Example:

<?php
namespace Vendor\Package;

class ClassName
{
    public function aVeryLongMethodName(
        ClassTypeHint $arg1,
        &$arg2,
        array $arg3 = []
    ) {
        // method body
    }
}

So, splitting in several lines is optional.


My personal vote is to not break arguments in several lines whatever their length.

Member

javiereguiluz commented May 26, 2016

As you know Symfony follows the PSR-2. In that standard we can read:

Argument lists MAY be split across multiple lines, where each subsequent line is
indented once. When doing so, the first item in the list MUST be on the next line,
and there MUST be only one argument per line.

When the argument list is split across multiple lines, the closing parenthesis and
opening brace MUST be placed together on their own line with one space between them.

Example:

<?php
namespace Vendor\Package;

class ClassName
{
    public function aVeryLongMethodName(
        ClassTypeHint $arg1,
        &$arg2,
        array $arg3 = []
    ) {
        // method body
    }
}

So, splitting in several lines is optional.


My personal vote is to not break arguments in several lines whatever their length.

@javiereguiluz javiereguiluz added the RFC label May 26, 2016

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire May 26, 2016

Contributor

Thanks for your answer, I know fully well this is optional (otherwise I guess you guys have tests for PSR2 that would not pass), but 500 chars, really, isn't that a bit too much ?

before :

public function __construct(TokenStorageInterface $tokenStorage, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfTokenManagerInterface $csrfTokenManager = null, SimpleFormAuthenticatorInterface $simpleAuthenticator = null) {
}

after

public function __construct(
    TokenStorageInterface $tokenStorage,
    AuthenticationManagerInterface $authenticationManager,
    SessionAuthenticationStrategyInterface $sessionStrategy,
    HttpUtils $httpUtils,
    $providerKey,
    AuthenticationSuccessHandlerInterface $successHandler,
    AuthenticationFailureHandlerInterface $failureHandler,
    array $options = array(),
    LoggerInterface $logger = null,
    EventDispatcherInterface $dispatcher = null,
    CsrfTokenManagerInterface $csrfTokenManager = null,
    SimpleFormAuthenticatorInterface $simpleAuthenticator = null
)

IMO, besides liking, habits or "readability", I think the latter is easier to review, especially if you're changing an argument, and also, easier to edit.

Plus, you can rapidly tell when a class probably does to much by seeing it has more than 10 arguments.

Contributor

greg0ire commented May 26, 2016

Thanks for your answer, I know fully well this is optional (otherwise I guess you guys have tests for PSR2 that would not pass), but 500 chars, really, isn't that a bit too much ?

before :

public function __construct(TokenStorageInterface $tokenStorage, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfTokenManagerInterface $csrfTokenManager = null, SimpleFormAuthenticatorInterface $simpleAuthenticator = null) {
}

after

public function __construct(
    TokenStorageInterface $tokenStorage,
    AuthenticationManagerInterface $authenticationManager,
    SessionAuthenticationStrategyInterface $sessionStrategy,
    HttpUtils $httpUtils,
    $providerKey,
    AuthenticationSuccessHandlerInterface $successHandler,
    AuthenticationFailureHandlerInterface $failureHandler,
    array $options = array(),
    LoggerInterface $logger = null,
    EventDispatcherInterface $dispatcher = null,
    CsrfTokenManagerInterface $csrfTokenManager = null,
    SimpleFormAuthenticatorInterface $simpleAuthenticator = null
)

IMO, besides liking, habits or "readability", I think the latter is easier to review, especially if you're changing an argument, and also, easier to edit.

Plus, you can rapidly tell when a class probably does to much by seeing it has more than 10 arguments.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz May 26, 2016

Member

Plus, you can rapidly tell when a class probably does to much by seeing it has more than 10 arguments.

That's why I prefer to keep these long lines. They are a painful indicator that something's wrong with that code (although sometimes that long code is right because it is just a rare edge case).

Member

javiereguiluz commented May 26, 2016

Plus, you can rapidly tell when a class probably does to much by seeing it has more than 10 arguments.

That's why I prefer to keep these long lines. They are a painful indicator that something's wrong with that code (although sometimes that long code is right because it is just a rare edge case).

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire May 26, 2016

Contributor

That's why I prefer to keep these long lines. They are a painful indicator that something's wrong with that code (although sometimes that long code is right because it is just a rare edge case).

Well if you see a ten line long constructor, you can also tell can't you ? But with more accuracy : you know how many arguments there are, regardless of how long their name, type hinting are. With the first method, you can't quickly tell.

Contributor

greg0ire commented May 26, 2016

That's why I prefer to keep these long lines. They are a painful indicator that something's wrong with that code (although sometimes that long code is right because it is just a rare edge case).

Well if you see a ten line long constructor, you can also tell can't you ? But with more accuracy : you know how many arguments there are, regardless of how long their name, type hinting are. With the first method, you can't quickly tell.

@Soullivaneuh

This comment has been minimized.

Show comment
Hide comment
@Soullivaneuh

Soullivaneuh May 26, 2016

Contributor

you know how many arguments there are, regardless of how long their name, type hinting are. With the first method, you can't quickly tell.

Or just read the phpdoc.

👍 to keep single line method/class signature. Keep single line method/class signatures is a question of clarity and consistence.

Contributor

Soullivaneuh commented May 26, 2016

you know how many arguments there are, regardless of how long their name, type hinting are. With the first method, you can't quickly tell.

Or just read the phpdoc.

👍 to keep single line method/class signature. Keep single line method/class signatures is a question of clarity and consistence.

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire May 26, 2016

Contributor

Consistency with what ? Existing code ? The point is to change it! As per clarity, see code snippet above, I can't believe how this is more clear to you.

Contributor

greg0ire commented May 26, 2016

Consistency with what ? Existing code ? The point is to change it! As per clarity, see code snippet above, I can't believe how this is more clear to you.

@Soullivaneuh

This comment has been minimized.

Show comment
Hide comment
@Soullivaneuh

Soullivaneuh May 26, 2016

Contributor

Please do not extract a word of my sentence. I'm also talking about clarity.

Yes this is a personal opinion, like using spaces instead of tabs. This is how standards are built: from opinions. 😉

Contributor

Soullivaneuh commented May 26, 2016

Please do not extract a word of my sentence. I'm also talking about clarity.

Yes this is a personal opinion, like using spaces instead of tabs. This is how standards are built: from opinions. 😉

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot May 26, 2016

Member

Indeed, we are not using multiline method signature, that's the choice we made and we won't change it as it would make merging branches a nightmare for no benefits.

Member

fabpot commented May 26, 2016

Indeed, we are not using multiline method signature, that's the choice we made and we won't change it as it would make merging branches a nightmare for no benefits.

@fabpot fabpot closed this May 26, 2016

@Soullivaneuh

This comment has been minimized.

Show comment
Hide comment
@Soullivaneuh

Soullivaneuh May 26, 2016

Contributor

@javiereguiluz in any case I agree with @greg0ire for one thing: This should be clarified on the coding standard document.

Contributor

Soullivaneuh commented May 26, 2016

@javiereguiluz in any case I agree with @greg0ire for one thing: This should be clarified on the coding standard document.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz
Member

javiereguiluz commented May 26, 2016

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire May 26, 2016

Contributor

@javiereguiluz in any case I agree with @greg0ire for one thing: This should be clarified on the coding standard document.

And maybe also explained.

Contributor

greg0ire commented May 26, 2016

@javiereguiluz in any case I agree with @greg0ire for one thing: This should be clarified on the coding standard document.

And maybe also explained.

xabbuh added a commit to symfony/symfony-docs that referenced this issue Jun 6, 2016

minor #6618 Added a note about coding standards and method arguments …
…(javiereguiluz)

This PR was submitted for the 2.3 branch but it was merged into the 2.7 branch instead (closes #6618).

Discussion
----------

Added a note about coding standards and method arguments

This coding standard is optional in the PSR-2 standard, so we must explain our own convention. Related to symfony/symfony#18890.

Commits
-------

b7429a5 Added a note about coding standards and method arguments

@greg0ire greg0ire referenced this issue Jun 13, 2016

Merged

Added abstract blocks #201

3 of 3 tasks complete
@sagikazarmark

This comment has been minimized.

Show comment
Hide comment
@sagikazarmark

sagikazarmark Jun 14, 2016

They are a painful indicator that something's wrong with that code (although sometimes that long code is right because it is just a rare edge case).

Indicator for whom: developers or users of the framework? Because it is definitely painful for everyone.

sagikazarmark commented Jun 14, 2016

They are a painful indicator that something's wrong with that code (although sometimes that long code is right because it is just a rare edge case).

Indicator for whom: developers or users of the framework? Because it is definitely painful for everyone.

@Soullivaneuh

This comment has been minimized.

Show comment
Hide comment
@Soullivaneuh

Soullivaneuh Jun 14, 2016

Contributor

Because it is definitely painful for everyone.

That's why this kind of long methods should be refactored in this case. 😉

I think this what @javiereguiluz tried to explain.

Contributor

Soullivaneuh commented Jun 14, 2016

Because it is definitely painful for everyone.

That's why this kind of long methods should be refactored in this case. 😉

I think this what @javiereguiluz tried to explain.

@sagikazarmark

This comment has been minimized.

Show comment
Hide comment
@sagikazarmark

sagikazarmark Jun 14, 2016

Well, for me the only logical reasoning is this by @fabpot:

it would make merging branches a nightmare for no benefits.

Although I don't think it is that dramatic (when there are no such long signatures in the first place) it makes sense and I don't think it requires more discussion. It is quite trivial after all.

sagikazarmark commented Jun 14, 2016

Well, for me the only logical reasoning is this by @fabpot:

it would make merging branches a nightmare for no benefits.

Although I don't think it is that dramatic (when there are no such long signatures in the first place) it makes sense and I don't think it requires more discussion. It is quite trivial after all.

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Jun 14, 2016

Contributor

That's why this kind of long methods should be refactored in this case. 😉

I think we get exactly what you mean, but this 500-chars-long character has been here for a while, and no one has done anything about it, so you might as well use a // FIXME comment or open an issue if you want to draw attention on that. Also, this alerting method is not accurate : you can get an alert if type hinting and arguments are long, even if you don't have many arguments:

<?php
function (Some\Looooooooooong\Classname $someVeryyyyyyyyyyyyyyyyyyyLongArgument, Some\Looooooooooong\Classname $someOtherVeryLongArgument)

I know you're going to say "but now you have another problem maybe, but the very same problem can also be seen with a multiline signatures

<?php
function (
    Some\Looooooooooong\Classname $someVeryyyyyyyyyyyyyyyyyyyLongArgument,
    Some\Looooooooooong\Classname $someOtherVeryLongArgument
)

Also, if you have both problems, you will see it more easily with a multi line signature :

<?php
function (
    $okayLengthArg,
    $okayLengthArg2,
    $okayLengthArg3,
    $okayLengthArg4,
    Some\Looooooooooong\Classname $someVeryyyyyyyyyyyyyyyyyyyLongArgument,
    Some\Looooooooooong\Classname $someOtherVeryLongArgument
)

But not so easily with a single-line signature

<?php
function ($okayLengthArg, $okayLengthArg2, $okayLengthArg3, $okayLengthArg4, Some\Looooooooooong\Classname $someVeryyyyyyyyyyyyyyyyyyyLongArgument, Some\Looooooooooong\Classname $someOtherVeryLongArgument
)
Contributor

greg0ire commented Jun 14, 2016

That's why this kind of long methods should be refactored in this case. 😉

I think we get exactly what you mean, but this 500-chars-long character has been here for a while, and no one has done anything about it, so you might as well use a // FIXME comment or open an issue if you want to draw attention on that. Also, this alerting method is not accurate : you can get an alert if type hinting and arguments are long, even if you don't have many arguments:

<?php
function (Some\Looooooooooong\Classname $someVeryyyyyyyyyyyyyyyyyyyLongArgument, Some\Looooooooooong\Classname $someOtherVeryLongArgument)

I know you're going to say "but now you have another problem maybe, but the very same problem can also be seen with a multiline signatures

<?php
function (
    Some\Looooooooooong\Classname $someVeryyyyyyyyyyyyyyyyyyyLongArgument,
    Some\Looooooooooong\Classname $someOtherVeryLongArgument
)

Also, if you have both problems, you will see it more easily with a multi line signature :

<?php
function (
    $okayLengthArg,
    $okayLengthArg2,
    $okayLengthArg3,
    $okayLengthArg4,
    Some\Looooooooooong\Classname $someVeryyyyyyyyyyyyyyyyyyyLongArgument,
    Some\Looooooooooong\Classname $someOtherVeryLongArgument
)

But not so easily with a single-line signature

<?php
function ($okayLengthArg, $okayLengthArg2, $okayLengthArg3, $okayLengthArg4, Some\Looooooooooong\Classname $someVeryyyyyyyyyyyyyyyyyyyLongArgument, Some\Looooooooooong\Classname $someOtherVeryLongArgument
)
@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Jun 14, 2016

Contributor

when there are no such long signatures in the first place

What do you mean? I gave an example from the symfony code of a 500 characters long method signature…

Contributor

greg0ire commented Jun 14, 2016

when there are no such long signatures in the first place

What do you mean? I gave an example from the symfony code of a 500 characters long method signature…

@Soullivaneuh

This comment has been minimized.

Show comment
Hide comment
@Soullivaneuh

Soullivaneuh Jun 14, 2016

Contributor

If you have something like this:

<?php
function (Some\Looooooooooong\Classname $someVeryyyyyyyyyyyyyyyyyyyLongArgument, Some\Looooooooooong\Classname $someOtherVeryLongArgument)

I think you have a problem too. 😛

Well, for me the only logical reasoning is this by @fabpot:

it would make merging branches a nightmare for no benefits.

Not the only IMHO but yes, this is the biggest issue.

Contributor

Soullivaneuh commented Jun 14, 2016

If you have something like this:

<?php
function (Some\Looooooooooong\Classname $someVeryyyyyyyyyyyyyyyyyyyLongArgument, Some\Looooooooooong\Classname $someOtherVeryLongArgument)

I think you have a problem too. 😛

Well, for me the only logical reasoning is this by @fabpot:

it would make merging branches a nightmare for no benefits.

Not the only IMHO but yes, this is the biggest issue.

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Jun 14, 2016

Contributor

Although I don't think it is that dramatic

The main problem is that every decision made here makes it to other project that simply base themselves on all symfony standards, which mean maybe the standards should be very very carefully chosen. And if something turns out to be not so great and changing it is too much of a hassle, just say that instead of carving the rule in stone. Something along the lines "Symfony does it like this, but this rule is a legacy rule, not part of the standards, follow it when contributing to Symfony only".

I think you have a problem too. 😛

Please read all of my answer before replying.

Fun Magento example (thanks @Soullivaneuh ) : https://github.com/magento/magento2/blob/a5be15b42bee194f5030cb516002aab5244fe057/app/code/Magento/Catalog/Model/Product.php#L392

Notice how you can see the problem without the signature being one-lined.

Contributor

greg0ire commented Jun 14, 2016

Although I don't think it is that dramatic

The main problem is that every decision made here makes it to other project that simply base themselves on all symfony standards, which mean maybe the standards should be very very carefully chosen. And if something turns out to be not so great and changing it is too much of a hassle, just say that instead of carving the rule in stone. Something along the lines "Symfony does it like this, but this rule is a legacy rule, not part of the standards, follow it when contributing to Symfony only".

I think you have a problem too. 😛

Please read all of my answer before replying.

Fun Magento example (thanks @Soullivaneuh ) : https://github.com/magento/magento2/blob/a5be15b42bee194f5030cb516002aab5244fe057/app/code/Magento/Catalog/Model/Product.php#L392

Notice how you can see the problem without the signature being one-lined.

@Soullivaneuh

This comment has been minimized.

Show comment
Hide comment
@Soullivaneuh

Soullivaneuh Jun 14, 2016

Contributor

Please read all of my answer before replying

I read it. This change not really nothing. You have two problems? Well, fix the first one and see what is going.

Contributor

Soullivaneuh commented Jun 14, 2016

Please read all of my answer before replying

I read it. This change not really nothing. You have two problems? Well, fix the first one and see what is going.

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Jun 14, 2016

Contributor

I read it. This change not really nothing. You have two problems? Well, fix the first one and see what is going.

That's the point : fixing the second might be way easier (if it is the argument name being to long), but if you can't notice it, too bad. That way you can be left with one hard problem instead of one hard problem and one easy problem.

Contributor

greg0ire commented Jun 14, 2016

I read it. This change not really nothing. You have two problems? Well, fix the first one and see what is going.

That's the point : fixing the second might be way easier (if it is the argument name being to long), but if you can't notice it, too bad. That way you can be left with one hard problem instead of one hard problem and one easy problem.

@Soullivaneuh

This comment has been minimized.

Show comment
Hide comment
@Soullivaneuh

Soullivaneuh Jun 14, 2016

Contributor

That's the point but I think it's pointless regarding the other inconvenients.

Your argument is acceptable, but I don't agree with it. :-)

Contributor

Soullivaneuh commented Jun 14, 2016

That's the point but I think it's pointless regarding the other inconvenients.

Your argument is acceptable, but I don't agree with it. :-)

@greg0ire

This comment has been minimized.

Show comment
Hide comment
@greg0ire

greg0ire Jun 14, 2016

Contributor

Your argument is acceptable, but I don't agree with it. :-)

Well let's agree to disagree then :)

Contributor

greg0ire commented Jun 14, 2016

Your argument is acceptable, but I don't agree with it. :-)

Well let's agree to disagree then :)

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 14, 2016

Member

Everything has been said, locking this conversation now.

Member

fabpot commented Jun 14, 2016

Everything has been said, locking this conversation now.

@symfony symfony locked and limited conversation to collaborators Jun 14, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.