Ternery operators

[photodude]

@mbabker raised a question in this PR About Ternery operators and allowing or disallowing multi-line use.

I think multi-line ternary operators could have a benefit, if they improve the readability of the code. I would definitely suggest disallowing nested or stacked ternary operators as those situations just create a mess for code readability and maintenance.

With that said; based on some things I have recently read, I question even allowing ternary operators. Or at least we should be strongly discouraging them.

In general I think if a code standard or code style guidelines achieve the following then they are probably something that should be considered for adoption.

• Does it improve code consistency?
• Does it improve code readability?
• Does it improve future maintainability?
• Does it improve code portability?
• Does it improve code performance?
• Does it improve code security?

So applying those questions to ternary operators:

• They are a deviation from standard if/else, thus creating a question of consistency.
• They generally reduce readability
• They generally reduce maintainability
• They can reduce performance (since ternary operators do a copy operation, as such larger data sets will cause more performance issues)
• As for portability, and security I don't know of any pros or cons.

I would also say that there are places where short hand code is discouraged or not allowed in the existing standard (PHP tags for instance). So this maybe another area where the short hand operators may not be the best choice, and should be disallowed or strongly discouraged.

[photodude]

The same "short hand use" question applies to short hand array declarations someArray = []; is a B/C issue for all packages still supporting PHP 5.3, those packages still need someArray = array();

[mbabker]

I don't know if there is a blanket answer for ternery operators. Things
like $foo = isset($bar) ? $bar : new Default; in a constructor read better
and are rather clear compared to doing a full if/else type construct. I do
agree it needs to be on simple operations only and things that turn complex
should be expanded to the full construct.

I don't think our code sniffer should get into disallowing syntax that
isn't supported on all core Joomla supported versions. The issue tracker
doesn't support PHP 5.3 so short array syntax is fine there. Likewise,
third party code may use the rules and not support PHP 5.3 (like some
extensions we have on Joomla.org sites). So that gets tricky as individual
pieces start dropping legacy support unless there is some way to inject
into the sniffer the minimum supported version for a package and adjust
from that.

On Monday, August 24, 2015, photodude notifications@github.com wrote:

The same "short hand use" question applies to short hand array
declarations someArray = []; is a B/C issue for all packages still
supporting PHP 5.3, those packages still need someArray = array();

—
Reply to this email directly or view it on GitHub
#119 (comment)
.

[photodude]

I agree there is no easy "blanket answer for ternery operators" If we take your example $foo = isset($bar) ? $bar : new Default; I can agree it is simpler and more readable for that instance type. But if $bar contains a large amount of data there could be a big performance hit. since ternary operators do a copy operation on their variables before returning the data.

Thinking about the wording, I think phrasing it in the following way might work as a general best practices in the code style but not necessarily a sniffer item:
Single construct ternery operators are allowed and can be multi-line for readability. Ternery operators should never be nested or stacked, you should always use full if/else type construct instead. Consider your variable's data before using a ternery operator. If your variable being evaluated by a Ternery operator contains, or could contain, a large amount of data; use a full if/else type construct instead to avoid performance issues.

The only thing I think the sniffer can do is allow or disallow multi-line Ternery operators. As long as we avoid nested or stacked Ternery operators I don't think multi-line Ternery operators are an issue.

Generally I agree "our code sniffer should get into disallowing syntax that
isn't supported on all core Joomla supported versions." But we already do disallow some short hand code declarations that don't apply to all things like the issue tracker. For example <?php vs <? this was an issue with some servers, since prior to php 5.4 you had to declare the short_open_tag variable to use it. Of course <? also causes conflicts with XML since <? is an XML deceleration. Since php 5.4 use of <?= or <?php shouldn't be an issue. Yet as a general code consistency issue in the code style (and to avoid issues that might pop up if a host happens to have the short_open_tag variable set to false) we specifically disallow the short forms of PHP's open tag.

There is a 3rd party code sniffer project that could help with the PHP versioning https://github.com/wimg/PHPCompatibility
It's a sniffer that checks for PHP version compatibility, it's something that I think could be used on a per project basis.

[wilsonge]

My opinion is also that single line ternary's are fine. Multi-line ternary's are not in terms of code style

[mbabker]

I think we should enforce that ternary is ONLY allowed for single line operations. But let's limit this to the 2.0 branch since it's kind of a B/C breaking change in the spec.

[wilsonge]

👍

[photodude]

Looks like the Pear standard allows for multiline Ternary and there is nothing existing in PHPCS that disallows multiline Ternary. (Squiz.WhiteSpace.OperatorSpacing kind of does, but I think it would mess up all multi-line operators) So we'll have to write a custom sniff to enforce that.

[photodude]

So Squiz.WhiteSpace.OperatorSpacing will disallow multi-line ternary, but it also disallows all multiline operators like + - * / so we'll have to write a custom sniff to enforce disallow multi-line ternary.

// Example usage of multi-line operators (this should still pass)
$test4 = $var1
        + $var2
        - $var3
        / $var4
        * $var6;

// Example usage for: Ternary Operator (this should pass)
$action = (empty($_POST['action'])) ? 'default' : $_POST['action'];

// Example of usage for multiline Ternary Operator (these should both fail)
$a = $condition1 && $condition2
    ? $foo : $bar;

$b = $condition3 && $condition4
    ? $foo_man_this_is_too_long_what_should_i_do
    : $bar;

[wilsonge]

If only the second one passed I wouldn't be hugely upset... especially if it makes things easier for you?

[photodude]

I'm not sure what it will take to write the custom sniff to completely disallow multi-line ternary or to only allow in the second format.

I'm probably going to put this on the back burner until some of the other items in the PHPCS 2 branch can be sorted out. I'm still trying to figure out what happened in that PR which caused the recent reported phpcbf failure with $instance = new $c($identifier); becoming $instance = new; I've tried replicating but can't reproduce.

[photodude]

I think something should be done in regards to a rule for these. but as I don't have the time or skills to address I will Suggest voting to close as a Stale issue.