David Deutsch wrote: > Alright, commit for the four spaces in the CSS files was added to the pull > request. > > Added a second PR that tackles four further plugins: > https://github.com/roundcube/roundcubemail/pull/110 > > So far, a couple of things came up in the discussion of the first PR: > > Biggest concern was use of braces and one-line statements. As I had written > earlier, I have two major rules that are in competition - First one is to > write as "properly" as possible, meaning braces go basically everywhere. > Second one is that if the situation allows for it, expressions should take > up as few lines as possible while staying below the 80 character limit. > Braces can be visual clutter here.
We started to add braces everywhere just to make the code consistent it simpler to add debug code in between. 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. > > That means we have a general conflict between "keep it as simple as > possible" and "keep it as concise as possible". As a first example, this is > where I join two lines into one: > > https://github.com/daviddeutsch/roundcubemail/commit/67426ea779c7c6d224b3947559dd9ae37b29b437 > https://github.com/daviddeutsch/roundcubemail/commit/08a50e4cb320bf6db66b9478ccfa3491d34053d7 > > I.E., in the first link, the $cache variable is used precisely one more > time and I don't think joining it into a one-liner makes the code less > readable (I would argue it makes it more readable, since everything is in > one place). Fully agreed. No variables needed if it's only used once (or none). > > As a counter-example, here, the expression rcmail::get_instance() is called > in six lines, making things overly complicated. Here, I declared one > $rcmail variable in the beginning and use that from there on. It actually > also helps reduce the line numbers later on: > > https://github.com/daviddeutsch/roundcubemail/commit/92666405bcc2068a52bb32ab4ed7220f55ae7e33 > > Getting to the more controversial stuff: I really like getting rid of > indents and long braced statements. Here is a small example: > > https://github.com/daviddeutsch/roundcubemail/commit/fee02cb397620922b94336a315b1089c6bef292d > > Everything is stuffed into one if statement. Instead, you can just do the > if statement and return; in case it isn't true. Single exit point of a function! Don't spread return statements all over. > It's not as obvious here, > but this comes in really handy when you have longer statements. So here is > a medium sized example: > > https://github.com/daviddeutsch/roundcubemail/commit/8ef53237ec2618b759cfc3ee5becd8ea12fd72e9#L2L214 > > The original ends up at four indents while my cleanup sits at one. (Well, > one and a half since I'm kinda-sorta cheating with the if one-liners.) The > control flow is a lot simpler to follow here: > > - Do we have print_to_console active? If not, return. > - Is it not an array? If yes, do something, then return. > - Otherwise check for a tag in the array - if yes, do something. Exception: return allowed at the start of a function if preconditions are not met. > > I end up with two more lines total, but I think the code is much, much more > readable: > > Original: > https://github.com/roundcube/roundcubemail/blob/master/plugins/debug_logger/runlog/runlog.php#L174 > Cleanup: > https://github.com/daviddeutsch/roundcubemail/blob/cleanup-2/plugins/debug_logger/runlog/runlog.php#L268 > > Things like this one will become even more apparent when I tackle longer > statements. Again: please no return statements in the middle of a function. ~Thomas _______________________________________________ Roundcube Development discussion mailing list [email protected] http://lists.roundcube.net/mailman/listinfo/dev
