I propose a change to code standards, concerning specific cases. This is intentionally posted here and not in the code standards repo, as I believe it will gain more attention and comments, here.
The changes affect:
implements
in class declarations, see PSR-2 here: http://www.php-fig.org/psr/psr-2/#4-1-extends-and-implementsIt improves readability when either the implements
or method arguments are many or the declaration in general is long.
Conforming to the current standard for method arguments, a method looks like this:
public static function getContentLanguages($checkPublished = true, $checkInstalled = true, $pivot = 'lang_code', $orderField = null,
$orderDirection = null)
{
The same code is more readable like this:
public static function getContentLanguages(
$checkPublished = true,
$checkInstalled = true,
$pivot = 'lang_code',
$orderField = null,
$orderDirection = null
){
Thank you for your comments and Happy New Year everybody!
Labels |
Added:
?
|
in favour
I hate it. This is why you invest in a decent IDE that makes it easier to view method signatures.
If you have a method signature that requires splitting across multiple lines then I would consider it a code smell and you should fix the underlying problem rather than try to make it look pretty.
@PhilETaylor That's a valid argument, but what about people that don't have such IDE's. Or if on the road with a tablet or a phone and wanting to do some urgent necessary changes? ...just throwing some edge cases in here for the sake of argument.
@chrisdavenport Agreed, but let's make them pretty first, to be easier to read and then work on code smell. The work to reduce code smell is a much tougher job in most cases. Let's try to make things better in steps. Cannot magically clean up the code base in on go. ;)
@PhilETaylor That's a valid argument, but what about people that don't have such IDE's. Or if on the road with a tablet or a phone and wanting to do some urgent necessary changes? ...just throwing some edge cases in here for the sake of argument.
Then they cannot call themselves professional developers then
Dare I say that professional developers would always use an IDE, even for urgent changes, especially for urgent changes, and if urgent code changes are needed then they should not be done on a phone or tablet anyway - you dont see a heart surgeon flip out his pocket knife...
The work to reduce code smell is a much tougher job in most cases.
With an IDE Refactoring Tool its a million times easier than you think.
I hate it. This is why you invest in a decent IDE that makes it easier to view method signatures.
By that argument arbitrary line length rules are pointless too (like our 150 character max that has zero flexibility at all)
If you have a method signature that requires splitting across multiple lines then I would consider it a code smell and you should fix the underlying problem rather than try to make it look pretty.
Yes and no. You could probably refactor to use an array holding all of the arguments, but personally I consider THAT a code smell over lengthy method signatures (you no longer have a properly declared signature, it's for all intents and purposes a magical black box). Any way you spin it though, the intent here is for readable code and right now the way our multi-line method signatures split is not very readable. This is more of a convenience thing in the long run anyway (same as aligning =>
in multi-line arrays or the equals sign when you have variable assignments in a row).
As for the IDE argument, IMO it is unfair to state that all code changes must be made in a fully functional development environment which includes an IDE tuned to that environment. In Joomla there are a lot of contributors who by your claims aren't "professional developers" because they use the GitHub web interface to make their contributions, does that make what they offer less valid than a change proposed by you or me?
150/160 char length was a standard before developers had wide screens and decent resolutions ;-)
Sent from my iPhone
On 1 Jan 2017, at 16:41, Michael Babker notifications@github.com wrote:
I hate it. This is why you invest in a decent IDE that makes it easier to view method signatures.
By that argument arbitrary line length rules are pointless too (like our 150 character max that has zero flexibility at all)
If you have a method signature that requires splitting across multiple lines then I would consider it a code smell and you should fix the underlying problem rather than try to make it look pretty.
Yes and no. You could probably refactor to use an array holding all of the arguments, but personally I consider THAT a code smell over lengthy method signatures (you no longer have a properly declared signature, it's for all intents and purposes a magical black box). Any way you spin it though, the intent here is for readable code and right now the way our multi-line method signatures split is not very readable. This is more of a convenience thing in the long run anyway (same as aligning => in multi-line arrays or the equals sign when you have variable assignments in a row).
As for the IDE argument, IMO it is unfair to state that all code changes must be made in a fully functional development environment which includes an IDE tuned to that environment. In Joomla there are a lot of contributors who by your claims aren't "professional developers" because they use the GitHub web interface to make their contributions, does that make what they offer less valid than a change proposed by you or me?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
It's still a good length because noody likes to read texts across the whole screen width :)
As for the multiline arguments, I don't care either way. However anything we do, should be supported by IDEs like PHPStorm so we don't have to manually adjust it each time.
The multi-line method arguments are supported by PHPStorm. The example above was formatted with PHPStorm 2016.3.2.
However PHPStorm currently has an issue with implements/extends, which is reported here: https://youtrack.jetbrains.com/issue/WI-24143 . So I guess, this might be a show-stopper for the multiline implements
. Pls vote for it, if you're interested.
There is also an issue for PHPDoc formatting and the multiple spaces in between the tag parameters. If enough Joomla PHPStorm users vote for that, they might implement it faster... https://youtrack.jetbrains.com/issue/WI-24129
I don't know about the other editors. Anyone?
I believe this kind of multiline alignment can be "automatically fixed with PHPCS 2.x if we decide to make a change to the Joomla code standards. (Check out the current PHPCS 2/3 version of the code standards check with auto fixers https://github.com/joomla/coding-standards/)
@photodude That would be great! Lets see how it goes.
I find this a bit of an awkward one. I agree with @PhilETaylor and personally use phpStorm it does a fantastic job of rendering the functions and making them readable. Also multiple lines means less code to fill the whole screen, so you are focusing on a tall but narrow screen with the above implementation, whereas most screens these days are widescreen which the exception of macs which tend to provide an exceptional amount of extra working space are the opposite way of this implementation. Therefore more code is in your view area which makes it easier to understand at a glance.
Even without an IDE you should be able to set your editor to highlight variables which make them stand out at a glance.
So I am therefore against this.
If you focus solely on an IDE environment, then honestly a lot of the code style rules don't apply. Remember though, the code doesn't only exist in the IDE. It is browsed here on GitHub, edited here on GitHub and through other "non-professional" tools, and if I had configured the phpDocumentor job to do so the code could also be visible inline on the API documentation site.
I don't think there's anything wrong with disagreeing with the idea, but personally, I disagree with the notion that the code should only be formatted to be consumed in an IDE. It's about it being consistently reviewable regardless of the environment. I can participate in code reviews on a mobile device while traveling when looking at a well formatted code base, this particular change makes that experience a little easier (and ya that might be an edge case but it goes to demonstrate there is more to the code than whether or not you can drop it in PhpStorm, Eclipse, or Sublime and the tooling makes everything pretty and usable).
Category | ⇒ | Code style |
Priority | Medium | ⇒ | Low |
Status | New | ⇒ | Discussion |
Closing this as all views have been expressed
Status | Discussion | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-12-25 14:53:35 |
Closed_By | ⇒ | brianteeman |
I'm all for it personally.