[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
Fri Aug 30 13:09:12 CEST 2013


Almost done with the third PR, again tackling 4 plugins (hey, almost
halfway through!).

Another good example came up during that. In this commit:
https://github.com/daviddeutsch/roundcubemail/commit/7c041eda2d68a59486574d32f939791a2ab313a1

Original:
https://github.com/roundcube/roundcubemail/blob/master/plugins/archive/archive.php#L13
Cleanup:
https://github.com/daviddeutsch/roundcubemail/blob/cleanup-3/plugins/archive/archive.php#L17

In the original, we have this long if/elseif/elseif statement spanning 40
lines, topped off with a hard to parse starter if. The funny thing is -
we're really only concerned with two cases to begin with, if the task is
'settings' or 'mail'. (Here, I'm actually not sure whether setting
self::$task to 'mail|settings' earlier means that only those two arrive at
the init() function to begin with... would make this even funnier...)

So I make that a condition right at the start - no other code in this needs
to be executed if we don't have that met. Next up, I handle the two smaller
cases first, to get them out of the way. I also pull $archive_folder into
its own line - this code is executed anyways, so doing it within the if
statement might be shorter, but also clutters up the statement and makes it
harder to understand. in_array() makes the if even shorter and we get a
nice, concise if statement for the second case.

Now that these are out of the way, we can simply run through the long if
body statement outside of the if, again making the code easier to parse.
(Granted, the syntax formatting already makes it easier to understand.)


On Fri, Aug 30, 2013 at 11:30 AM, David Deutsch <skoremail at gmail.com> wrote:

> Last link should be:
> https://github.com/daviddeutsch/roundcubemail/blob/cleanup-2/plugins/debug_logger/runlog/runlog.php#L242
>
>
> On Fri, Aug 30, 2013 at 10:57 AM, David Deutsch <skoremail at gmail.com>wrote:
>
>> Alright, commit for the four spaces in the CSS files was added to the
>> pull request.
>>
>> Added a second PR that tackles four further plugins:
>> https://github.com/roundcube/roundcubemail/pull/110
>>
>> So far, a couple of things came up in the discussion of the first PR:
>>
>> Biggest concern was use of braces and one-line statements. As I had
>> written earlier, I have two major rules that are in competition - First one
>> is to write as "properly" as possible, meaning braces go basically
>> everywhere. Second one is that if the situation allows for it, expressions
>> should take up as few lines as possible while staying below the 80
>> character limit. Braces can be visual clutter here.
>>
>> That means we have a general conflict between "keep it as simple as
>> possible" and "keep it as concise as possible". As a first example, this is
>> where I join two lines into one:
>>
>>
>> https://github.com/daviddeutsch/roundcubemail/commit/67426ea779c7c6d224b3947559dd9ae37b29b437
>>
>> https://github.com/daviddeutsch/roundcubemail/commit/08a50e4cb320bf6db66b9478ccfa3491d34053d7
>>
>> I.E., in the first link, the $cache variable is used precisely one more
>> time and I don't think joining it into a one-liner makes the code less
>> readable (I would argue it makes it more readable, since everything is in
>> one place).
>>
>> As a counter-example, here, the expression rcmail::get_instance() is
>> called in six lines, making things overly complicated. Here, I declared one
>> $rcmail variable in the beginning and use that from there on. It actually
>> also helps reduce the line numbers later on:
>>
>>
>> https://github.com/daviddeutsch/roundcubemail/commit/92666405bcc2068a52bb32ab4ed7220f55ae7e33
>>
>> Getting to the more controversial stuff: I really like getting rid of
>> indents and long braced statements. Here is a small example:
>>
>>
>> https://github.com/daviddeutsch/roundcubemail/commit/fee02cb397620922b94336a315b1089c6bef292d
>>
>> Everything is stuffed into one if statement. Instead, you can just do the
>> if statement and return; in case it isn't true. It's not as obvious here,
>> but this comes in really handy when you have longer statements. So here is
>> a medium sized example:
>>
>>
>> https://github.com/daviddeutsch/roundcubemail/commit/8ef53237ec2618b759cfc3ee5becd8ea12fd72e9#L2L214
>>
>> The original ends up at four indents while my cleanup sits at one. (Well,
>> one and a half since I'm kinda-sorta cheating with the if one-liners.) The
>> control flow is a lot simpler to follow here:
>>
>> - Do we have print_to_console active? If not, return.
>> - Is it not an array? If yes, do something, then return.
>> - Otherwise check for a tag in the array - if yes, do something.
>>
>> I end up with two more lines total, but I think the code is much, much
>> more readable:
>>
>> Original:
>> https://github.com/roundcube/roundcubemail/blob/master/plugins/debug_logger/runlog/runlog.php#L174
>> Cleanup:
>> https://github.com/daviddeutsch/roundcubemail/blob/cleanup-2/plugins/debug_logger/runlog/runlog.php#L268
>>
>> Things like this one will become even more apparent when I tackle longer
>> statements.
>>
>> cheers,
>> David
>>
>>
>> On Thu, Aug 29, 2013 at 5:50 PM, David Deutsch <skoremail at gmail.com>wrote:
>>
>>> > Hmm, we have 4 spaces everywhere else (and in Larry skin we have tabs
>>> I suppose). I'd either go for tabs or 4 spaces as well.
>>>
>>> Agreed, I'll switch it to four spaces.
>>>
>>>
>>> >  One more thing: I can already see ourselves in merging hell when we
>>> want to  merge other pull requests or development branches back into master
>>> because  most likely a merge attempt will result in conflicts.
>>>
>>> Yeah, that was my main concern as well. I've checked through the current
>>> PRs and it seems like none of them touch the plugins. Since I'm working on
>>> those first, that's fine - you should have a bit of time to merge those in
>>> until I'm done. If there are PRs that cannot be resolved quickly, we might
>>> have to check on them on a case-by-case basis. If it's just a couple of
>>> files, I can always roll back changes and redo them, although obviously I'd
>>> like to avoid that, if possible. As for branches - I only saw the release
>>> branches. Do you mean local dev branches?
>>>
>>> cheers,
>>> David
>>>
>>>
>>> On Thu, Aug 29, 2013 at 5:40 PM, Thomas Bruederli <thomas at roundcube.net>wrote:
>>>
>>>> David Deutsch wrote:
>>>> > First pull request is up:
>>>> https://github.com/roundcube/roundcubemail/pull/109
>>>>
>>>> Thanks! We'll read through it.
>>>> >
>>>> > There were a couple of things that might turn out to be
>>>> controversial, I
>>>> > think mostly with calling on the html class - those seem to have a
>>>> tendency
>>>> > to get out of hand, so I decided to split it over multiple lines.
>>>> >
>>>> > For CSS, I went with a two spaces indent, which seems to be the most
>>>> > commonly used in the codebase. Let me know if you want four spaces
>>>> instead.
>>>>
>>>> Hmm, we have 4 spaces everywhere else (and in Larry skin we have tabs I
>>>> suppose). I'd either go for tabs or 4 spaces as well.
>>>>
>>>> One more thing: I can already see ourselves in merging hell when we
>>>> want to
>>>> merge other pull requests or development branches back into master
>>>> because
>>>> most likely a merge attempt will result in conflicts. Any suggestions
>>>> how
>>>> that could go smooth? Maybe we should first clean up all of our pending
>>>> PRs
>>>> and try to eliminate existing dev branches before the cleanup really
>>>> gets
>>>> started.
>>>>
>>>> Kind regards,
>>>> Thomas
>>>>
>>>> _______________________________________________
>>>> 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/20130830/236c22e2/attachment.html>


More information about the dev mailing list