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

Thomas Bruederli thomas at roundcube.net
Fri Sep 6 13:52:21 CEST 2013


On Thu, Sep 5, 2013 at 12:11 PM, David Deutsch <skoremail at gmail.com> wrote:
>> I really hate negated expressions and mid-block returns but this is not my
>> code I just wrote some plugins to fit RC in my environment.
>
> The way I'm applying them right now, I don't particularly enjoy them either,
> but I do think they're the lesser of two evils. As discussed before - what
> you want to have, in the end, is a separation into functions. I think
> disentangling those large if statements is a good first step towards that.

OK, after long discussions here and in the pull requests on github,
can we try to conclude where that refactoring is supposed to go?

For what we have seen and discussed, here's what I would like to
happen and not to happen:

* no intermediate returns but clear if/elseif/else clauses. If the
individual clauses exceed a certain number of lines (let's say, 20
lines) they should become individual functions/methods.

* allow immediate returns at the beginning of a function when
pre-conditions are not met.

* only re-arrange blocks of variable assignments or function call
blocks with additional lines where they're really hard to understand.

e.g. these should not be torn apart with blank lines:

    $rcmail = rcmail::get_instance();
    $identity = $rcmail->user->get_identity();
    $identities_level = intval($rcmail->config->get('identities_level', 0));

* no one-line if blocks without braces.

* all if/for/while blocks should get braces.

* else if / else clauses start on a new line. Not "} else {"

* object/array function arguments should start on the same line as the
function call. That saves an indentation level.

Good:
    dialog.dialog({
        ...
    });

   $this->api->output->button(array(
       ...
    ));

Bad:
    dialog.dialog(
        {
            ...
        }
    );

   $this->api->output->button(
       array(
           ...
       )
    );

* Javascript: opening brace for anonymous functions are on the same
line. No space between 'function' and brackets.

---------

May I ask you to re-do or update the pending pull requests according
to this suggestion? And in order to save your precious time, you
should wait for approval of the first pull request before moving on.

Thanks a lot for you efforts and all the inputs!

Regards,
Thomas


More information about the dev mailing list