joomla / coding-standards

Joomla Coding Standards Definition
https://developer.joomla.org/coding-standards/basic-guidelines.html
GNU General Public License v2.0
128 stars 129 forks source link

Proposal for rule to consistently use `&&` and `||` instead of `AND` and `OR` operators. #191

Closed frankmayer closed 5 years ago

frankmayer commented 7 years ago

As discussed here: https://github.com/joomla/joomla-cms/pull/12233 - starting at review: https://github.com/joomla/joomla-cms/pull/12233#pullrequestreview-2379665, it is better to have a standard way to use logical operators - namely && and || -, instead of having the differently behaving AND and OR, especially in template files. This should put a correct standard in place.

Bakual commented 7 years ago

I prefer to have and and or in the layout files because they are easier to read and understand for users. The layout files are the files which are supposed to be edited by users when they want to make an override and adjust the output. We even offer that possibility in the backend.

As for coding style, it should be added in the section "Mixed language usage (e.g. at the layout files)". We already specify to use the alternative syntax (if, elseif, endif) for the same reason.

As for being interchangeable: The use of the operators in the layout files are quite simple cases (and should be, for the aformentioned reason). I honestly would be surprised if there is a place in a layout file where they would be not interchangeable.

frankmayer commented 7 years ago

A few points, to clarify:

1) I believe that if a user gets to play around with template or layout files, which are of mixed type (PHP and HTML and maybe also JS), the user should know at least the basics of the programming language.

2) To use something that might produce different results from what is expected, is - especially for "novice" users - confusing.

3) To use and recognize && and || is really not too much to ask from a user. JS uses those, too. So even if you're a front-end developer without knowing PHP, you know && and ||.

4) I intentionally did not put that in the "Mixed" section, as it should be a general rule. With emphasis on the layouts / templates.

5) Template- and layout-files might get more complicated in future and I do believe that we should not pull the handbrakes on templates just to be easier to read for "novice" users. As explained above, users who consider modifying their templates should know (or be willing to learn) how stuff works.

6) The use of && and || is generally considered best practice. AND and OR can easily return unexpected results. Try to explain to those users (that you want to make things easier for, by keeping AND and OR), the notion of precedence in that matter. Even more seasoned programmers sometimes have to think hard to wrap their heads around code written that way. Grouping expressions with parentheses instead, is clearer and leaves no doubt.

Bakual commented 7 years ago

I believe that if a user gets to play around with template or layout files, which are of mixed type (PHP and HTML and maybe also JS), the user should know at least the basics of the programming language.

Maybe he should, but certainly not all do and it is not a requirement. Keep in mind we offer an editor in our backend which allows any admin to adjust a layout. We should make it as simple as possible for them.

To use something that might produce different results from what is expected, is - especially for "novice" users - confusing.

Do you have an example in our codebase where it would produce a different result? It's only a few cases where that could happen and it is not something we usually have in layout files.

To use and recognize && and || is really not too much to ask from a user. JS uses those, too. So even if you're a front-end developer without knowing PHP, you know && and ||.

Yep, but and and or is still simpler. And there is no backend editor for JS files. We're not talking about frontend developers here. We talk about admins which very often neither know PHP nor JS.

Template- and layout-files might get more complicated in future and I do believe that we should not pull the handbrakes on templates just to be easier to read for "novice" users. As explained above, users who consider modifying their templates should know (or be willing to learn) how stuff works.

Actually, that would be a wrong way to go. layout files should get simpler and have les logic in them, certainly not more.

Try to explain to those users (that you want to make things easier for, by keeping AND and OR), the notion of precedence in that matter.

I don't have to explain it because as written already to my knowledge we don't have that complex conditionals in layout files where it would matter. Also, parentheses can be used regardless to make to code easier to read.

frankmayer commented 7 years ago

Actually, that would be a wrong way to go. layout files should get simpler and have les logic in them, certainly not more.

I know and agree, but I have no clear vision of how Joomla's layouts/templating will develop in future. So that was only an example. Want to call it a bad one? No issue, However, most PHP template engines do support complicated stuff in templates for whatever strange reason one may encounter, sometimes leaving best practices aside. That was my thinking for that example.

Yep, but and and or is still simpler. And there is no backend editor for JS files. We're not talking about frontend developers here. We talk about admins which very often neither know PHP nor JS.

A few more points: 1) There is no backend editor for JS files "now". This might change. 2) At the moment it is a mixture of both ways. This is also not in the interest of the "users". If the community chooses to go for a specific in mixed cases, all should change to one specific method. 3) As soon as you have a "user" installing a third party template or extension with override-able template files or layout files, the whole thing is up to the 3rd party developer anyways. So in any case, in my opinion Joomla should at least set a standard, so that 3rd party developers also follow a consistent way of doing things (if they choose to). It makes things easier for everybody.

Bakual commented 7 years ago

There is no backend editor for JS files "now". This might change.

It may, but I doubt it.

At the moment it is a mixture of both ways. This is also not in the interest of the "users". If the community chooses to go for a specific in mixed cases, all should change to one specific method.

It is currently not defined in the codestyle, but personally I would follow the same rule as we do with the "alternate syntax". Eg within the "mixed content" part of the layout files we use "and" and "or" and in other places we use "&&" and "||". Mostly it is already like this as far as I remember.

As soon as you have a "user" installing a third party template or extension with override-able template files or layout files, the whole thing is up to the 3rd party developer anyways.

Coding standards don't apply to 3rd party extensions anyway. They always do what they want :) But yeah having it specified in the coding standard may convince some of them to do the same (however they likely already do today)

frankmayer commented 7 years ago

It is currently not defined in the codestyle, but personally I would follow the same rule as we do with the "alternate syntax". Eg within the "mixed content" part of the layout files we use "and" and "or" and in other places we use "&&" and "||". Mostly it is already like this as far as I remember.

Just to clarify, do you mean to leave keep the mixed (both methods) way in the current builtin template files? (In essence not standardizing and use only one? (A mixture does confuse the "users", too ;) )

photodude commented 7 years ago

The code standards PHPCS checks actually does enforce && and || for logical operators. Not sure why it's not explicitly stated in the manual.

frankmayer commented 7 years ago

@photodude Does it also do so for the mixed files?

Bakual commented 7 years ago

Not sure why it's not explicitly stated in the manual.

Template files are excluded from PHPCS to my knowledge.

Just to clarify, do you mean to leave keep the mixed (both methods) way in the current builtin template files? (In essence not standardizing and use only one? (A mixture does confuse the "users", too ;) )

You mean the "top" part vs the "lower" part with the actual HTML output? Honestly I don't care much. The top part is the one which is usually not adjusted in overrides and it's usually the logic which shouldn't be in the layout file to begin with 😄

zero-24 commented 7 years ago

Template files are excluded from PHPCS to my knowledge.

Correct ;) the tmpl folder is exclued :)

photodude commented 7 years ago

PHPCS does have issues with mixed php/html files such as those used for the CMS view layouts. The CMS explicitly excludes those view files (tmpl folders) for checking coding standards (since only manual checking and compliance enforcement happens, very low compliance actually happens).

@Bakual I did a quick glance at the PHP code section of the manual and I didn't see a specified statement on logical operators in the control structures section of the manual. We still enforce || and && even if the manual is missing a statement saying so.

photodude commented 7 years ago

bump for merge consideration based on existing enforcment /cc @mbabker @wilsonge

Bakual commented 7 years ago

bump for merge consideration based on existing enforcment

It's fine for regular PHP files. However I don't think we should enforce it for layout files. This PR would explicitely include layout files.

Honestly, I don't see a reason to include the layout files so they use the || and &&. Consistency is no argument as we already use the alternative syntax for control structures so the code is easier to read for non-coders. Same applies for or and and imho. The difference in precedence doesn't matter as well since we don't have such statements in those files.

photodude commented 7 years ago

layout files are excluded from the automatic checks in the CMS. So it's only enforced "if" reviewers enforce it. Use of and and or in the layout files is not an affirmation of using the alternative syntax, but evidence that the existing expectation was not enforced by reviewers since there is no automated check.

I'm for code consistency, so always enforcing || and &&. I feel that consistency helps with avoiding rule confusion rather than having exceptions that may confuse people. my two cents.

frankmayer commented 7 years ago

@mbabker, @wilsonge any thoughts on this?

frankmayer commented 6 years ago

Bump @mbabker, @wilsonge any thoughts on this?

mbabker commented 6 years ago

Late to the party, but...

I'd say && and || should be preferred over and and or, but I wouldn't go so far as creating a hard rule forbidding the latter pair. There are cases where their use can make sense (i.e. code clarity) without impacting functionality and I don't think there's a reason to force those valid cases to be replaced. Mostly, this is going to be in layout focused files (and FWIW you have templating engines like Twig which actually don't have && and || operators but use more "human friendly" syntax).

wilsonge commented 6 years ago

I kinda agree with @Bakual I'd hard require && and || in pure PHP files and honestly I'd probably require and and or in the layout files so it's simpler for users. Effectively already we have different rules for layouts vs our PHP classes files anyway

photodude commented 6 years ago

Note: Twig layout files like base.html.twig wouldn't be checked by our code standards as only CSS, JS, and PHP files can be checked by our sniffs or the core PHPCS sniffs. Some sniffs only do JS, and PHP. In our case, only PHP files ever get checked since we hard specify that on the Command line.

As noted our mixed PHP/HTML layout files are excluded in most of the CMS code style checks.

frankmayer commented 6 years ago

So, what's the verdict on this PR? Any chance to get anything of this into the codestyle definitions or should I close it?

frankmayer commented 5 years ago

I am closing this one, since the verdict seems to be to not favor code consistency in favour of more "human friendly" syntax. If need be, we can reopen this. Thanks for your thoughts everyone. :smile: