[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
Mon Sep 2 21:20:57 CEST 2013


> A proper and predictable process flow is something I learned to
appreciate during my years as a software developer.

Sure, although I don't understand how my approach makes it any less
predictable. All I have done is fan out the if/elseif structures and,
arguably, that makes it easier to check through them. Maybe changing it to
no-oneliner-ifs will make this a little clearer for you, but the return
statements really stand out when you scroll through the code - whether or
not they are wrapped in if blocks or individually.

> What's wrong about if clauses with proper indenting? These can even be
collapsed with a sexy IDE.

Nothing wrong with them. It's just: Do you wrap the entire content of a
function in an if or do you make a negated if -> return statement? The
first doesn't scale, the second makes things more readable - that was
really all I was trying to convey.

> If the structure gets too complicated, individual blocks should maybe be
moved to individual functions with sensible names.

Precisely my point ;-) What I'm doing right now is cleanup to make the
existing functions easier to read. If adding functions was on the table, my
work would be very different, of course.

-David


On Mon, Sep 2, 2013 at 9:13 PM, Thomas Bruederli <thomas at roundcube.net>wrote:

> On Sun, Sep 1, 2013 at 8:08 PM, David Deutsch <skoremail at gmail.com> wrote:
> >> And what's this 80 characters discussion anyways? Who still has a screen
> >> that only fits 80 characters? Line breaks should be added where it makes
> >> sense but please don't take these 80c as the holy limit.
> >
> > Yeah, well, as I've said: It's really more of a personal preference of
> mine.
> > For my own code, I have found that trying to see 80 lines as a semi-holy
> > limit can help to force simplify my approach.
> >
> > It's obviously no longer about small screens (also my biggest pet peeve
> > about people dismissing tabs as indents because some exotic strawman IDEs
> > can't deal with them properly).
> >
> >
> >> Single exit point of a function! Don't spread return statements all
> over.
> >
> > Would need a clarification here. There are tons and tons of examples in
> the
> > original code where this is not the case. My approach is "Handle corner
> > cases first, then the big pile of code" - so with the examples, get a
> couple
> > of returns out of the way, then do what the function should usually do.
>
> I didn't say that our codebase is free of these but I'd not want more
> intermediate returns to be added but rather getting rid of them. A
> proper and predictable process flow is something I learned to
> appreciate during my years as a software developer.
> >
> > As an extreme example: You could start a function with an if structure
> that
> > checks whether all arguments of the function are the correct type. But
> you
> > wouldn't want that to create a five level indent - you want to handle
> them
> > in a structure like the way I set them up and then move on to the actual
> > code.
>
> What's wrong about if clauses with proper indenting? These can even be
> collapsed with a sexy IDE. If the structure gets too complicated,
> individual blocks should maybe be moved to individual functions with
> sensible names.
>
> >> Exception: return allowed at the start of a function if preconditions
> are
> >> not met.
> >
> > Yeah, I guess we are in agreement about that, then ;-)
>
> Great!
>
> > Will move on to the GitHub comments now.
>
> Aye!
>
> ~Thomas
>
>
> > On Sun, Sep 1, 2013 at 7:20 PM, Thomas Bruederli <thomas at roundcube.net>
> > wrote:
> >>
> >> David Deutsch wrote:
> >> > Thanks.
> >> >
> >> > And find them I did!
> >> >
> >> > It's just that Thomas said earlier they were the standard in the
> >> > codebase
> >> > and me changing them would confuse existing developers. So I remain
> >> > unconvinced there is what I'm saying... ;-)
> >>
> >> I obviously got your intentions wrong and I'm sorry about this.
> >> We definitely want if ($sky == 'blue') and not if ('blue' == $sky).
> >>
> >> ~Thomas
> >>
> >> >
> >> >
> >> > On Sat, Aug 31, 2013 at 10:43 PM, Cor Bosman <cor at xs4all.nl
> >> > <mailto:cor at xs4all.nl>> wrote:
> >> >
> >> >
> >> >     On Aug 31, 2013, at 2:00 PM, David Deutsch <skoremail at gmail.com
> >> >     <mailto:skoremail at gmail.com>> wrote:
> >> >
> >> >>     Another possibly neat comparison:
> >> >>
> >> >>     Original:
> >> >>
> >> >>
> https://github.com/roundcube/roundcubemail/blob/master/plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php#L206
> >> >>     Cleanup:
> >> >>
> >> >>
> https://github.com/daviddeutsch/roundcubemail/blob/37167c5ce1c00cb4f42b7f59a9ff56b81b3cd874/plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php#L219
> >> >>
> >> >>     The code may be /slightly/ less DRY, but, I find, a lot more
> >> >> readable.
> >> >
> >> >     Definitely more readable.
> >> >
> >> >>
> >> >>     I must also say that so far, I find very few yoda conditions! ;-)
> >> >
> >> >     Find them you will.
> >> >
> >> >     Cor
> >> >
> >> >
> >> >     _______________________________________________
> >> >     Roundcube Development discussion mailing list
> >> >     dev at lists.roundcube.net <mailto:dev at lists.roundcube.net>
> >> >     http://lists.roundcube.net/mailman/listinfo/dev
> >> >
> >> >
> >> > _______________________________________________
> >> > Roundcube Development discussion mailing list
> >> > dev at lists.roundcube.net
> >> > http://lists.roundcube.net/mailman/listinfo/dev
> >> _______________________________________________
> >> Roundcube Development discussion mailing list
> >> dev at lists.roundcube.net
> >> http://lists.roundcube.net/mailman/listinfo/dev
> >
> >
> >
> > _______________________________________________
> > Roundcube Development discussion mailing list
> > dev at lists.roundcube.net
> > http://lists.roundcube.net/mailman/listinfo/dev
> _______________________________________________
> 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/20130902/5aa78d7e/attachment.html>


More information about the dev mailing list