> 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:

1. 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:
> 1. 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

> 2. (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.

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