?
avatar frankmayer
frankmayer
31 Dec 2016

Subject of proposal

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:

Why

It 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! ?

avatar frankmayer frankmayer - open - 31 Dec 2016
avatar joomla-cms-bot joomla-cms-bot - change - 31 Dec 2016
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - labeled - 31 Dec 2016
avatar frankmayer frankmayer - change - 31 Dec 2016
The description was changed
avatar frankmayer frankmayer - edited - 31 Dec 2016
avatar mbabker
mbabker - comment - 31 Dec 2016

I'm all for it personally.

avatar jeckodevelopment
jeckodevelopment - comment - 31 Dec 2016

in favour

avatar PhilETaylor
PhilETaylor - comment - 1 Jan 2017

I hate it. This is why you invest in a decent IDE that makes it easier to view method signatures.

avatar chrisdavenport
chrisdavenport - comment - 1 Jan 2017

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.

avatar frankmayer
frankmayer - comment - 1 Jan 2017

@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. ;)

avatar PhilETaylor
PhilETaylor - comment - 1 Jan 2017

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

avatar mbabker
mbabker - comment - 1 Jan 2017

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?

avatar PhilETaylor
PhilETaylor - comment - 1 Jan 2017

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.

avatar Bakual
Bakual - comment - 1 Jan 2017

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.

avatar frankmayer
frankmayer - comment - 1 Jan 2017

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?

avatar photodude
photodude - comment - 2 Jan 2017

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/)

avatar frankmayer
frankmayer - comment - 2 Jan 2017

@photodude That would be great! Lets see how it goes.

avatar tonypartridge
tonypartridge - comment - 3 Jan 2017

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.

avatar mbabker
mbabker - comment - 3 Jan 2017

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

avatar nibra nibra - change - 24 Mar 2017
Category Code style
avatar nibra nibra - change - 24 Mar 2017
Priority Medium Low
Status New Discussion
avatar brianteeman
brianteeman - comment - 25 Dec 2017

Closing this as all views have been expressed

avatar brianteeman brianteeman - change - 25 Dec 2017
Status Discussion Closed
Closed_Date 0000-00-00 00:00:00 2017-12-25 14:53:35
Closed_By brianteeman
avatar brianteeman brianteeman - close - 25 Dec 2017

Add a Comment

Login with GitHub to post a comment