On Saturday, September 7, 2013, David Deutsch wrote:
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
I'm sorry but I can't follow you here. What we see here is the processing of different cases in individual if/elseif blocks on the same indentation level. What's so wrong about this? IMO your optimizations are way less readable because they add an additional indentation level (which you aim to avoid with intermediate returns, don't you?) So except the reformatting of the arguments array of raise_error() calls, I definitely don't see anything that needs to be improved in order to make the code more understandable to others, sorry.
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.
You might be right with the simple examples above and I partially agree that this waists some vertical space with single-line if clauses. But we want to make our code consistent, right? And I think the extra line breaks generally contribute to the readability because they visually separate the blocks which is desirable in the average case.
On the one hand you suggest that variable assignments and such should be torn apart with empty lines in between for better readability and on the other hand you don't accept our way to visually separate if/else blocks by moving the elseif/else statement to a new line?
(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)
Yes, it might not fit the PSR-2 guidelines but come on, if somebody is unable to read our code because of that, he/she would not qualify as a useful contributor to Roundcube anyways. I know, I'm probably risking to become an arrogant dick on your eyes but I'm just naming the facts.
- 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
It's definitely not meant to cover flaws and we already agreed that too big blocks should be refactored into functions or in another meaningful way. But that's not an argument for or against the said new lines.
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 ;-)
I'm willing to bear some pain when I see a benefit. But I more and more have the feeling that we got ourselves into a bikeshed discussion. And I'd therefore appreciate other opinions on this. If I'm really the only dickhead who hates the PSR-2 if/else syntax, I'll step back.
I would usually invert the if and in that "return $fields['user'];". Would that be permissible?
I tend to say no on this. But again, I'd like to hear others speaking up for or against single exit points. For me these are like GOTO commands. They suddenly make the program jump out of the current block which - especially if it's a function - has a clear beginning and an end.
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?
I previously tried to give a limit of 20 lines for if/else blocks. But that was just a wild estimation without looking at specific examples. But since Alec vetoed on function refactoring, there's no point in stating a number right now.
Thanks a lot for your patience in discussing this though!
Regards,
Thomas