Chuck Burgess [2008-03-07 16:57 UTC] These all look good to me. In particular are the changes to mitigate the effects of the 85-char line width CS rule. In my CS cleanup work in PhpDocumentor, this line width rule alongside all the current spacing rules has proven the hardest to repair. The proposed changes will greatly ease this.
Philippe Jausions [2008-03-07 17:10 UTC] Good to update the CS for these oft-encountered long lines.
My comments by section:
- "Split function definitions onto several lines". Not a big fan of ) { on the same line. I'd rather see the ) on the same line as the last parameter, and leave the { on its own line.
- "Split long if statements onto several lines". For better readability expressions in parentheses should be indented by 1 for each grouping. i.e. (Note: I'm not sure how that'll look on the comment once posted though)
if (($condition1
|| $condition2) <-- indent 6 spaces here
&& ($condition3 <-- indent 5 spaces here
&& $condition4) <-- indent 9 spaces here
) { <-- indent 4 spaces here (?)
//code here
}
Jonathan Street [2008-03-07 17:11 UTC] I think the alignment of assignments is perhaps overly strict. Particularly in comparison to the alignment of function parameters. So to compare,
"To support readability, parameters in subsequent calls to the same function/method **may** be aligned by parameter name:"
with
"To support readability, the equal signs **should** be aligned in block-related assignments:"
Emphasis is mine.
David Jean Louis [2008-03-07 17:22 UTC] I fully agree with the proposal :)
I also think that alignment of assignations should not be forced (PHPCodeSniffer throws a WARNING atm).
I also like the possibility to call functions like this:
someFunction(
$param1,
$param2
);
When the param is an array:
someFunction(array(
'foo' => 'bar',
'spam' => 'eggs'
));
And when there's an array and other vars:
someFunction(
array(
'foo' => 'bar',
'spam' => 'eggs'
),
$param2
);
Of course, PHP_CodeSniffer will have to be updated if the proposal is accepted.
Chuck Burgess [2008-03-07 18:00 UTC] We should be cognizant of where it currently states "must be" or "are to be" (required) versus where it states "may be" or "can be" (allowed).
I think many of the ideas came about as ways of dealing with the 85-char limit (? is it 80 or 85 ?) combined with Codesniffer's rigid interpretation of spacing inside parentheses and after commas. "No space after open parens", "only one space allowed after comma", ... I don't think these are explicit restrictions named in the CS, but they are implied by the code examples. Combine these with long code lines and the 85-char width limit, and you get haphazardly creative to meet all the restrictions.
In some of my own work, the only way I could split long lines without violating those two spacing rule examples above was to split method-naming calls on the arrow:
-| $myLongObjectName->myExtraLongMethodName($firstArg, $secondArg);
becomes:
-| $myLongObjectName->
-| myExtraLongMethodName($firstArg, $secondArg);
Had the CS _explicitly_ allowed me some leeway in spacing, I could instead use the "function arguments can be on separate lines" option, in any of these ways:
-| $myLongObjectName->myExtraLongMethodName(
-| $firstArg, $secondArg
-| );
==========
-| $myLongObjectName->myExtraLongMethodName(
-| $firstArg,
-| $secondArg
-| );
So, to me, the key to us changing the CS boils down to how we _state_ these "rules" vs "allowed possibilities". By opening up some new "allowed possibilities", we solve the "_implied_ rigid spacing" issue we have in our current CS.
In the case of personal preference colliding with some of the 4-space indention usage, maybe our "requirement" can be "at least 4" while "allowing more spacing for personal preference readability". It seems to me that in these cases, lack of at least 4 spaces is what hinders readability, not necessarily that exactly 4 reads better than maybe 6 or 8 given a particular example.
Matthew Weier O'Phinney [2008-03-07 18:37 UTC] I agree with all, with the same exception as Philippe Jausions -- I'd prefer that the ending paren be on the same line as the condition/parameter, and the opening paren by itself on the following line. This makes it easier to visually see the start and end of a block as the braces align.
So,
if ($var
&& $foo
|| $bar)
{
}
and:
public function callSomeRidiculouslyLongMethodName(
$param1
$param2)
{
}
Till Klampaeckel [2008-03-07 19:01 UTC] I have a general question.
Will your proposed changes "open the coding standard" so people are able to relax a bit, or will your proposal imply new rules which add CS errors to all existing code which has been free of CS issues up until now.
Christian Weiske [2008-03-07 19:09 UTC] Response to Till:
The enhancements I propose extend our possibilities. Many of the suggested changes are listed because the current CS do not allow such code layouts.
Paul Jones [2008-03-07 19:15 UTC] I have a suggestion on the "how-to" of splitting long if() conditions. I think we should encourage that the conditions be expressed as variables, and then use those variables in the if() condition, so as to keep the condition set all on one line.
Given the original example, that would translate into something like the following:
$is_foo = ($condition1 || $condition2);
$is_bar = ($condition3 && $condtion4);
if ($is_foo && $is_bar) {
// ....
}
This has the benefit of "naming" the condition sets so they are easier to distinguish, and splits the different condition sets into readable chunks. Doing this should greatly reduce the need for splitting if() condition sets across multiple lines.
Michael Gauthier [2008-03-07 20:48 UTC] Is there a reason for putting the operators at the beginning of the next line instead of the end of the previous line?
I don't mind much one way or the other and I do want a defined standard on way or the other Just curious why this style:
if ($condition1
&& $condition2
) {
// code
}
was chosen over
if ($condition1 &&
$condition2
) {
// code
}
Christian Weiske [2008-03-07 21:00 UTC] Answer to Michael:
I chose
> if ($condition1
> && $condition2
over
>if ($condition1 &&
> $condition2
because you can easily comment-out conditions in the first version, which is really helpful when debugging or programming at the source location in general.
Philippe Jausions [2008-03-07 22:40 UTC] Might want to add something regarding ternary operator as well then...
$a = ($test)
? $yes
: $no;
David Sanders [2008-03-08 01:57 UTC] I already do all of these suggestions anyway, mostly to meet the archaic line length rule. However aligning blocks of code is something I can't live without ;) In addition to aligning assignment statements you might also want to think about arrays that have been split over several lines:
class Foo
{
public $some_really_long_array = array(
'option_a' => 'blah',
'foo' => 'bar',
... etc ...
}
...and of course splitting regular arrays with indentation.
With regards to splitting method calls, I like Chuck's idea of putting the method name on the next line.
But my question here is why do we still need 85 characters as the limit? Is it because code might get emailed? We're not living in the 1980's anymore... so viewing and printing don't seem to be an issue. This came about again for me with File_Infopath when I started to add Xpaths which pushed the line length right out past 85 chars. It was much more readable to have it all on the one line rather than concatenate strings together on separate lines.
Travis Swicegood [2008-03-11 15:19 UTC] Sorry for coming to the party late, but I thought I would throw my two cents worth in. I have gone through periods where I have aligned function parameters and arrays for readability. I do like the output. Without fail, however, I end up having to modify 5 lines of code because I added one new key/value pair to an array where the key was 2 characters longer than the other keys. This messes up diffs and makes changes appear heavier than they are.
Every time I have tried using alignment either in assigning values, arrays, or function calls, I end up reverting back to my old ways out of frustration. Maybe I just rewrite my code too often for it to be useful, but I would personally pull all references to alignment as they can get in the way and serve next to no valid purpose. In this instance, I'm balking at "readability" being a valid purpose as any decent IDE will give you much more usable UI into the code than any amount of formatting can.
I do like the formalized indenting rules, however. I use those on a daily basis already, so having them becoming "official" would be nice.
Another nice addition would be multi-line formatting and indenting of arrays (from my perspective without alignment, obviously). Having a trailing commas with the closing parenthesis would be nice as well as it helps keep the diff minimal.
$some_array = array(
'foo',
'bar',
);
Michael Gauthier [2008-03-11 15:42 UTC] I'm also in favor of a trailing comma on the last item in a multi-line array. Like putting the conditional operators at the beginning of the line in multiline conditionals, it would make debugging easier.
Chuck Burgess [2008-03-12 13:59 UTC] Now that it's been highlighted, I have to agree on the trailing comma idea for arrays. I've had originally been surprised by PHP's parser allowing this rather than considering a syntax error like other languages would, and it does indeed ease temporary commenting-out of individual lines without causing syntax errors.
For years I've been in the habit of dropping my commas to the beginning of the next line, specifically because of this, more often with SQL editing. Too bad this can't help me in that respect ;)
Jeff Moore [2008-03-15 23:39 UTC] I agree that keeping lines under 80 characters is important. However, I think it is possible for a coding standard to be over-specific. Maybe the mechanics of splitting lines is over-specification?
That said...
Split function definitions onto several lines:
If you are going to use more than one line, why not put each parameter on its own line?
Also, I think the preferred choice is not to have so many parameters. See Martin Fowler's Refactoring book for the code smell "long parameter list."
Rather than specify how to properly format the code smell, I'd rather see avoidance when possible and embarrassment when not.
Split function call on several lines:
Same goes here. one per line when necessary. Avoiding long parameter lists avoids long call lists. Use temporary "explaining" variables and "explaining methods" instead of complex formatting. (See Kent Beck's implementation patterns book as well as the chapter in the Refactoring book on making method calls simpler.)
Split long if statements onto several lines:
While not listed as a code smell in the refactoring book , complex conditionals should have been there. (It is defined as a smell in Refactoring to Patterns). Again, the solution is simplify conditional, use explaining variables, etc. See the Chapter in the refactoring book on making conditionals simpler.
Alignment of function parameters:
Alignment of assignments:
I've always found this kind of alignment to be more work than its worth. Is PHP_CodeSniffer really going to warn on these? I sure hope not. These are best as optional or personal preference, and thus not in the specification of standards.
Summary:
Rather than document the proper way to format hard-to-understand code fragments (code smells) and rather than burden the standard with hopefully rare edge cases, I think its best to leave them undocumented and thus slightly uncomfortable to use and slightly discouraged.
I think specifying PHP_CodeSniffer edge cases dilutes the usefulness of the standard for the cases that matter more. I don't want to have to become a code formatting lawyer when I'm coding.
Matthew Weier O'Phinney [2008-03-16 12:38 UTC] While I agree with Jeff in large part, there are a number of cases where breaking across multiple lines is not simply a case of code smell. As an example, you will often want to pass a previously undefined array or hash to a function call; being able to break it across multiple lines makes the code more readable. Having some standards for these situations will make for more readable, consistent code, and make for one less decision the developer needs to make when they find themselves in such a situation.
Helgi Þormar Þorbjörnsson [2008-03-16 13:21 UTC] To answer Jeff a bit, this will be thought more of as a recommended practice in X situation and not pure standard, not sure how the code sniffer deals with that, via warnings perhaps ?
The point being, I think it's better we show people via examples and recommended ways how to solve these code smells since we can't really expect them to read couple of patterns books before submitting code ;-)
That being said I'd like to include a reference to said books and chapters in the manual part that talks about these code smelling beautification, kinda like "if you are in situation X then look at these books and patterns but if things fail then at least try to make your code more readable by using any of these formatting approaches"
I also think it's important we don't say one way is the golden way, providing multiple examples per each category Christian listed, like people have been doing, this way people can more pick according to their style.
David Jean Louis [2008-08-24 19:12 UTC] Great work Christian,
Just a note: I would make "Alignment of assignments" completely optional, regardless of the number of spaces.
For the rest: amen.
Till Klampaeckel [2008-08-24 23:45 UTC] Great work, Christian. :)
Depending on the out come of the next meeting, do you think you can include the recent discussion about allowing prefixing of protected methods with an underscore?
Stefan Walk [2008-08-25 13:33 UTC] The split function definition part is contradicting the current standard, where the opening brace of the function definition is on its own line.
Daniel O'Connor [2008-11-03 07:17 UTC] From staring at PHPCS results for a long long time; the biggest areas where people fall down:
1) "@param string $param Oops, did you just put more than one space param or your variable"
Kind of pointless; most documentation generators can deal with it.
2) Forgetting to put in /** @return null */ or void when a method doesn't actually return things
3) File comments & Class comments having to duplicate a lot of the required tags; and get the indentation right.
IE:
/**
* File.php
*
* ... snip ...
*
* @category Fishing
* @author Dave <dave@dave.com>
*/
... snip ..
/**
* File_Class
*
* ... snip ...
*
* @category Fishing
* @author Dave <dave@dave.com>
*/
... will yell at you for incorrectly doing to docblocks.
Daniel O'Connor [2008-11-03 07:20 UTC] Oops!
1) "@param string $param Oops, did you just put more than one space after your param or your variable"
... even
Dan Scott [2008-11-03 15:45 UTC] Daniel's comments on all 3 issues look like they were written after running phpcs against my code (242 errors, 142 warnings) - very familiar.
I would be a big +1 to at least relaxing the level of enforcement on these things to "warning" rather than "error". It was irritating to spend about an hour cleaning up the "errors" in File_MARC that made no visible difference to the documentation (okay - there were a handful of incorrect parameter names in the docblocks that were valuable and rightfully called errors) or to the quality of the code.
I'm concerned that enforcing the coding standards for whitespace at the level of "error" will push potential PEAR developers away. We tend to have a whole lot of other things to do; reformatting code is a pretty low priority.
|
