Hi Thomas,
Yes, that was what I was trying to get at with my last post - wrapping up the decisions so we can move along ;-)
I agree with all of the points raised except one - having else/elseifs always start on a new line. First, let me tell you where I agree with you - there really are some "long" if/else blocks (exceeding a dozen or so lines)
- for those, I would agree that in the current state of the codebase,
pulling apart the structure a bit with new lines can help readability. In the end, I think the best way to go would be a terse (no linebreaks) syntax and (again in agreement with what you wrote) extra methods, but Alec has vetoed new methods during cleanup, so I cannot go there at the moment. Of course, I'd be more than happy to have that option, after all ;-)
But, again - I can't help myself, so still trying to argue here. In my opinion, a linebreak like that, applied consistently, would absolutely give other developers working with the codebase pause and even if we assumed for the sake of argument that the general readability in both possible scenarios is more or less equal, in a worst case scenario, the linebreaks route is simply more confusing. My go-to example would be this:
https://github.com/roundcube/roundcubemail/blob/master/plugins/managesieve/l... vs. https://github.com/daviddeutsch/roundcubemail/blob/905ef62978970e27818656610...
Of course, there are a couple of other things going on here that also don't help (mixing braced and non-braced multiline ifs etc.), but even during my cleanup, I had a very hard time even getting an angle at understanding that construct. Especially in cases like this one, it would be very hard to defend this decision as anything but a code smell.
Now, you might rightfully say that this is an extreme example that needs to be solved differently anyhow. But that gets back to my earlier point - the majority of if/else statements already are terse enough to be understood at one glance, examples:
https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f... https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f... https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f... https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f... https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f...
I don't see how the extra linebreaks would improve the situation here. So following that reasoning, we end up with fewer and fewer cases where the newline really makes sense... and thus I would think it also makes less and finally no sense as a general rule. It's either ill advised or so obscure that it could just as well be personal taste.
(I would also like to add the curiosity that some of the above examples are precisely the same in the original code - just with added brackets. ;-) )
To sum up:
- Extremely uncommon syntax (while I'm trying to change the codebase into
being more approachable to outsiders)
- In the few situations where it is helpful, it mostly covers other flaws
(code smells like overly long functions)
- In most situations, it's not helpful at all
And once again - I'm not trying to force your hand here or anything and I think I have shown that I'm usually absolutely happy to accept an opinion contrary to my own and incorporate it. In this case however, I'm pretty sure it's not just my taste against yours. My - I think informed - opinion is that you're making a bad call on this. I further think that one or two passes down the cleanup, these won't matter nearly as much as they seem to matter now.
Finally - it's a cleanup, some things are supposed to hurt. That's just the way it is. Part ye with ye olde ways and such ;-)
For the "no returns in the middle of a function" requirement, I would like to have one clarification. As an example, this function here ends with a rather long if and a small else:
https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f...
I would usually invert the if and in that "return $fields['user'];". Would that be permissible?
In general, I'm having a hard time finding the right ranges here. Maybe we could set a rough limit on length of function or maybe you could give me one or two examples from the existing cleanups that illustrate the point better?
regards, David
On Fri, Sep 6, 2013 at 1:52 PM, Thomas Bruederli thomas@roundcube.netwrote:
On Thu, Sep 5, 2013 at 12:11 PM, David Deutsch skoremail@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 _______________________________________________ Roundcube Development discussion mailing list dev@lists.roundcube.net http://lists.roundcube.net/mailman/listinfo/dev