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/lib/Roundcube/rcube_sieve_engine.php#L425
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/20de28b9e94a5f001161ca49f0cba183a504a014/plugins/acl/acl.php#L92
https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f0cba183a504a014/plugins/acl/acl.php#L418
https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f0cba183a504a014/plugins/acl/acl.php#L541
https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f0cba183a504a014/plugins/acl/acl.php#L570
https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f0cba183a504a014/plugins/acl/acl.php#L657
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/20de28b9e94a5f001161ca49f0cba183a504a014/plugins/acl/acl.php#L353
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?