[RCD] Codebase cleanup, PSR-1/2 compliance - and before you say no, I'm offering to do the work

David Deutsch skoremail at gmail.com
Tue Sep 3 12:51:15 CEST 2013


> 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 at 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 at lists.roundcube.net
> http://lists.roundcube.net/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.roundcube.net/pipermail/dev/attachments/20130903/bf1be728/attachment.html>


More information about the dev mailing list