Hi Woody, This is a good example of "code smell". Reducing the usage of `else` and > `elseif` greatly helps code readability. >
That is a valid point. Consider this, though: long list of elseif branches is very common when lexing or parsing something. That code also is performance sensitive so refactoring should be done cautiously. Here is an example from my own code: https://github.com/sad-spirit/pg-builder/blob/master/src/sad_spirit/pg_builder/Lexer.php#L594 And to push the point a bit further, here is an example from twig's code: https://github.com/twigphp/Twig/blob/2.x/src/Lexer.php#L295 OK, there are no blank lines in Twig's code, but elseif statements are moved below the closing brace of the previous branch to group related code together (and separate unrelated): // operators if (preg_match($this->regexes['operator'], $this->code, $match, null, $this->cursor)) { $this->pushToken(/* Token::OPERATOR_TYPE */ 8, preg_replace('/\s+/', ' ', $match[0])); $this->moveCursor($match[0]); } // names elseif (preg_match(self::REGEX_NAME, $this->code, $match, null, $this->cursor)) { $this->pushToken(/* Token::NAME_TYPE */ 5, $match[0]); $this->moveCursor($match[0]); } So a couple questions: - Will readability of the above code improve once you make it conform to PSR-12? - Is there an easy way to reduce the number of elseif / else branches? понедельник, 15 апреля 2019 г., 20:41:41 UTC+3 пользователь Woody Gilk написал: > > > Otherwise once you have even a couple of short elseif's you end up with > a pretty long statement where no blank lines are allowed. > > This is a good example of "code smell". Reducing the usage of `else` and > `elseif` greatly helps code readability. > -- > Woody Gilk > https://shadowhand.me > > > On Mon, Apr 15, 2019 at 11:09 AM <alexey...@gmail.com <javascript:>> > wrote: > >> Hi Alexander, >> >> > You are allowed to separate related blocks of code inside >> if/elseif/else: >> >> Yes, but block2 in your example MUST be stuck to unrelated elseif, if one >> follows. Why?.. >> >> I'd suggest relaxing this rule and only disallow blank line before the >> final closing brace in if / elseif / else and try / catch / finally >> statements. Otherwise once you have even a couple of short elseif's you end >> up with a pretty long statement where no blank lines are allowed. >> >> > It seems that current version doesn't forbid placing blank lines after >> opening brace of the control structure so the following is allowed: >> >> In fact phpcs complains about this if you run with PSR12 standard. >> However I can't find the relevant part in the standard either... >> >> > That, I think, is not good. >> >> Agree here. >> >> >> понедельник, 15 апреля 2019 г., 16:38:07 UTC+3 пользователь Alexander >> Makarov написал: >>> >>> > So the question is: why am I allowed to add blank lines to indicate >>> related blocks of code inside switch statements, but explicitly forbidden >>> to do that inside if / elseif / else statements (and probably try / catch / >>> finally, didn't check what phpcs has to say about these)? >>> >>> You are allowed to separate related blocks of code inside if/elseif/else: >>> >>> if ($something) { >>> // block1 >>> // block1 >>> >>> // block2 >>> // block2 >>> } >>> >>> It seems that current version doesn't forbid placing blank lines after >>> opening brace of the control structure so the following is allowed: >>> >>> if ($something) { >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> // hello >>> } >>> >>> That, I think, is not good. >>> >>> >>> On Sunday, April 14, 2019 at 11:26:05 PM UTC+3, alexey...@gmail.com >>> wrote: >>> >>>> Hi Alexandru, >>>> >>>> > And to answer your second question, the example with extra new lines >>>> before closing brace makes the code less readable, for me at least. >>>> >>>> Yeah, maybe you like your code with no blank lines whatsoever. However >>>> the standard *usually* allows adding blank lines, quoting section 2.3: >>>> > Blank lines MAY be added to improve readability and to indicate >>>> related blocks of code except where explicitly forbidden. >>>> >>>> So the question is: why am I allowed to add blank lines to indicate >>>> related blocks of code inside switch statements, but explicitly forbidden >>>> to do that inside if / elseif / else statements (and probably try / catch >>>> / >>>> finally, didn't check what phpcs has to say about these)? >>>> >>>> The reasons not to allow blank lines after opening '{' and before last >>>> '}' I can understand easily and agree with these. >>>> >>>> > this format was decided long time ago >>>> >>>> I am asking about the "new" proposed PSR-12, not "long ago" PSR-2, in >>>> case you missed. The whole point of updating a standard is to address >>>> inconsistencies in its previous version, don't you agree? >>>> >>>> >>>> воскресенье, 14 апреля 2019 г., 22:29:45 UTC+3 пользователь Alexandru >>>> Pătrănescu написал: >>>>> >>>>> Hi Alexey, >>>>> >>>>> And to answer your second question, the example with extra new lines >>>>> before closing brace makes the code less readable, for me at least. >>>>> >>>>> As you might think, how easy is to read a text is different from >>>>> person to person and when this format was decided long time ago, multiple >>>>> already written code from major frameworks or libraries were taken in >>>>> consideration >>>>> >>>>> What you can do is to work with the standard way and you will get use >>>>> to it, I guess. >>>>> >>>>> On another topic, you can avoid using `else` for an if condition to >>>>> make code readable. >>>>> Or avoid `if` conditions by using polymorphism. >>>>> >>>>> Regards, >>>>> Alex >>>>> >>>>> On Sun, Apr 14, 2019 at 9:54 PM Woody Gilk <wood...@gmail.com> wrote: >>>>> >>>>>> The middle example, without extra blank lines, is correct. >>>>>> -- >>>>>> Woody Gilk >>>>>> https://shadowhand.me >>>>>> >>>>>> >>>>>> On Sun, Apr 14, 2019 at 12:07 PM <alexey...@gmail.com> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> I tried running recent phpcs on my code base using --standard=PSR12 >>>>>>> and among the obvious errors there was a less obvious one: "Blank line >>>>>>> found at end of control structure". >>>>>>> >>>>>>> This was pointing to the blank lines in if / else / elseif >>>>>>> structures: >>>>>>> if ($foo) { >>>>>>> doFoo(); >>>>>>> >>>>>>> } elseif ($bar) { >>>>>>> doBar(); >>>>>>> >>>>>>> } else { >>>>>>> doSomethingElse(); >>>>>>> } >>>>>>> >>>>>>> I checked the text of the proposed standard and it states in section >>>>>>> 5: >>>>>>> >>>>>>> - The closing brace MUST be on the next line after the body >>>>>>> >>>>>>> So I have two questions >>>>>>> >>>>>>> - Is phpcs correct in interpreting the standard? Should the >>>>>>> brace before the elseif or else branch be considered "closing"? >>>>>>> - If it is correct, doesn't the rule go somewhat against the >>>>>>> goal of making the code more readable? >>>>>>> >>>>>>> I mean, removing the blank lines yields >>>>>>> if ($foo) { >>>>>>> doFoo(); >>>>>>> } elseif ($bar) { >>>>>>> doBar(); >>>>>>> } else { >>>>>>> doSomethingElse(); >>>>>>> } >>>>>>> >>>>>>> The above code can also be rewritten as switch where the standard >>>>>>> generously allows blank lines between case statements >>>>>>> switch (true) { >>>>>>> case $foo: >>>>>>> doFoo(); >>>>>>> break; >>>>>>> >>>>>>> case $bar: >>>>>>> doBar(); >>>>>>> break; >>>>>>> >>>>>>> default: >>>>>>> doSomethingElse() >>>>>>> } >>>>>>> but IMO this is also harder to understand. >>>>>>> >>>>>>> -- >>>>>>> You received this message because you are subscribed to the Google >>>>>>> Groups "PHP Framework Interoperability Group" group. >>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>> send an email to php...@googlegroups.com. >>>>>>> To post to this group, send email to php...@googlegroups.com. >>>>>>> To view this discussion on the web visit >>>>>>> https://groups.google.com/d/msgid/php-fig/5a7b2e01-36d2-4ad5-b2b2-1db11f4777ba%40googlegroups.com >>>>>>> >>>>>>> <https://groups.google.com/d/msgid/php-fig/5a7b2e01-36d2-4ad5-b2b2-1db11f4777ba%40googlegroups.com?utm_medium=email&utm_source=footer> >>>>>>> . >>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>> >>>>>> -- >>>>>> You received this message because you are subscribed to the Google >>>>>> Groups "PHP Framework Interoperability Group" group. >>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>> send an email to php...@googlegroups.com. >>>>>> To post to this group, send email to php...@googlegroups.com. >>>>>> To view this discussion on the web visit >>>>>> https://groups.google.com/d/msgid/php-fig/CAGOJM6J0%3D82Ocovwr5EVujmSTPL_m%2BTor0p_Rbn8Tn9QKyX2QQ%40mail.gmail.com >>>>>> >>>>>> <https://groups.google.com/d/msgid/php-fig/CAGOJM6J0%3D82Ocovwr5EVujmSTPL_m%2BTor0p_Rbn8Tn9QKyX2QQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>> . >>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>> >>>>> -- >> You received this message because you are subscribed to the Google Groups >> "PHP Framework Interoperability Group" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to php...@googlegroups.com <javascript:>. >> To post to this group, send email to php...@googlegroups.com >> <javascript:>. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/php-fig/d0ca55a4-ac96-42c1-9b5e-ec7b0d03bcc2%40googlegroups.com >> >> <https://groups.google.com/d/msgid/php-fig/d0ca55a4-ac96-42c1-9b5e-ec7b0d03bcc2%40googlegroups.com?utm_medium=email&utm_source=footer> >> . >> For more options, visit https://groups.google.com/d/optout. >> > -- You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group. To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+unsubscr...@googlegroups.com. To post to this group, send email to php-fig@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/cebe0b33-ea2a-4302-9ac4-e75ca40724c9%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
