I'd add newline only before comment line or if assignment is very
different (in look and logic).
Agreed. While I do the single-line-if-removal thing, I will keep an eye out for assignment blocks. If you have time, feel free to mark instances that you want me to look at in the PRs.
And I suppose this change comes from your hard 80-chars line length
limit. So, please, just don't be so strict with the line length limit.
No, that wasn't 80 char limit (as I've said before - that's a personal preference for my own stuff, not something I use in this cleanup effort).
In this instance, I simply think that the code is easier to read. Just for reference, the difference is between:
foreach ((array)$rcmail->config->get('identity_select_headers', array()) as $header) {
and
$headers = (array) $rcmail->config->get('identity_select_headers', array())
foreach ($headers as $header) {
I don't care about char limit - the original is just plain hard to parse. Now, that has two reasons:
- The config uses a getter with a default instead of a magic function
where variable defaults are stored separately. 2. ->get() apparently cannot be trusted to return the variable type that we need. So we cast an array.
What you really want instead of the above is this:
foreach ($rcmail->config->identity_select_headers as $header) {
To make that happen, you simply make a magic __get() function that uses a lookup table determining that if the variable is there, return it while making sure it is an array. Otherwise return a default - an empty array. Of course, a lookup table also is kind of a bandaid, what you really want is absolutely consistent storage of parameters - identity_select_headers should never be anything BUT an array. But that might be an unrealistic goal, at least for the short term. In the end, I think such a construct would help start a process where you end up with consistent (environment/configuration) variable use throughout the system.
I just found it to be much more readable if there's a new line. Also, I'd
like to put comments before else, not inside the block. So, if we need temporary solution, please add linebreaks.
See, that's my main gripe: Putting a comment between a control statement and a bracket. If an elseif needs a comment so you can understand what's going on, then THAT is the problem - not where you put the comment. There was one instance where I cleared up an if/elseif block to the extent where each elseif was very, very simple. The original state of the code relied on comments to explain the code - I'd rather have the code explain itself.
Standards with newline before else
FuelPHP has everything on a new line, not just else/elseif blocks. I don't know what PHP Developer is (have a link?) and Drupal... Well... They also use 2 spaces for indents. Sorry, but they really just have terrible coding standards. (Don't get me started on Wordpress 3PD extensions ;-) .)
-David
On Tue, Sep 3, 2013 at 11:31 AM, A.L.E.C alec@alec.pl wrote:
On 09/03/2013 10:34 AM, David Deutsch wrote:
- Linebreaks separating variable declaration blocks
Discussion:
https://github.com/roundcube/roundcubemail/pull/109#discussion-diff-6064999
I think for this, we will end up reverting some of the changes that I
made.
But we still have to discuss the details - It's just mainly about the
other
devs telling me in what instances it would be permissible to separate blocks.
Vertical spacing is also important and I think we should not make the code to be stretched too much. In my opinion we should keep such variable assignment lines together as soon as they are looking/doing similar things or they are a simple assignments like $var = array(); I'd add newline only before comment line or if assignment is very different (in look and logic).
Do not forget about equal sign alignment rule, http://pear.php.net/manual/en/rfc.cs-enhancements.alignassignments.php
- (very minor) One-time use variable declarations
Discussion:
https://github.com/roundcube/roundcubemail/pull/114#discussion-diff-6102900
Only happened once so far, I think. Not sure where Thomas stands on it
now.
And I suppose this change comes from your hard 80-chars line length limit. So, please, just don't be so strict with the line length limit.
- Linebreaks before ifelse / else statements
Discussion:
https://github.com/roundcube/roundcubemail/pull/114#discussion-diff-6102891
I must say this is really the only one that I feel strongly that the codebase should break with tradition because I think that doing it this
way
makes it very hard to follow the code. There is also no standard I have seen so far that encourages linebreaks there (I know, I know, appeal to popularity). In most cases, I think what everybody would prefer is proper separation into individual functions or rewriting the structure in some other way, so maybe Alec and Thomas will accept that removing the linebreaks is a temporary thing until we arrive at something better.
I just found it to be much more readable if there's a new line. Also, I'd like to put comments before else, not inside the block. So, if we need temporary solution, please add linebreaks.
Standards with newline before else: Drupal, PHP Developer, FuelPHP. So, really, it's not our invention.
-- Aleksander 'A.L.E.C' Machniak LAN Management System Developer [http://lms.org.pl] Roundcube Webmail Developer [http://roundcube.net]
PGP: 19359DC1 @@ GG: 2275252 @@ WWW: http://alec.pl _______________________________________________ Roundcube Development discussion mailing list dev@lists.roundcube.net http://lists.roundcube.net/mailman/listinfo/dev