David Deutsch wrote:
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/7c041eda2d68a59486574d3...
Original: https://github.com/roundcube/roundcubemail/blob/master/plugins/archive/archi... Cleanup: https://github.com/daviddeutsch/roundcubemail/blob/cleanup-3/plugins/archive...
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.)
Generally we should not have return statements spread throughout a function. A function should have one single exit point at the end. Everything else is hard to debug where you start wondering yourself why the hell the code doesn't hit your break point.
Exceptions are allowed right at the start of a function where some preconditions checks are made with an immediate exit if they're not met.
I'll let you get away with this here because the init() function doesn't really return anything. But please bear that in mind: one single entry point, one single exit point.
~Thomas