Moodle Community Sites
  1. Moodle Community Sites
  2. MDLSITE-1766

We need coding style guidance for the ternary operator

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Component/s: Coding style
    • Labels:
      None
    • Rank:
      39844

      Description

      a) When its acceptable to use it (is it always acceptable? i've always stayed away from it in Moodle).
      b) What whitespacing should be - e.g. is this ok:

      $params['limit'] = (!empty($CFG->navcourselimit))?$CFG->navcourselimit:20;
      

        Activity

        Hide
        Dan Poltawski added a comment - - edited

        Another example, which I think is particularly hard to read:

        $toload = (empty($CFG->navshowallcourses))?self::LOAD_ROOT_CATEGORIES:self::LOAD_ALL_CATEGORIES;
        
        Show
        Dan Poltawski added a comment - - edited Another example, which I think is particularly hard to read: $toload = (empty($CFG->navshowallcourses))?self::LOAD_ROOT_CATEGORIES:self::LOAD_ALL_CATEGORIES;
        Hide
        Sam Hemelryk added a comment -

        Hi Dan,

        Its a good question and certainly worth clarifying.
        I'm not a big fan of ternary operators but do use them occasionally (hehe given those are both from nav code).
        I remember discussing it in the past and the outcome being that they are OK as long as they are simple, only single level (definitely no nested ternary assignments) that are not part of any other operation.

        Would I miss them if they were no longer allowed? Absolutely not.
        Perhaps it is easier to simply say no to them, than to come up with rules.

        Also worth noting/considering is that people use them for the most basic operations as well.

        $row = ($i % 2)?'odd':'even';
        

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Dan, Its a good question and certainly worth clarifying. I'm not a big fan of ternary operators but do use them occasionally (hehe given those are both from nav code). I remember discussing it in the past and the outcome being that they are OK as long as they are simple, only single level (definitely no nested ternary assignments) that are not part of any other operation. Would I miss them if they were no longer allowed? Absolutely not. Perhaps it is easier to simply say no to them, than to come up with rules. Also worth noting/considering is that people use them for the most basic operations as well. $row = ($i % 2)?'odd':'even'; Cheers Sam
        Hide
        Tim Hunt added a comment -

        I think that they should almost never be used.

        If they are used, there should be whitespace:

        $row = ($i % 2) ? 'odd' : 'even';

        (After all, we require whitespace around +, - etc, and they are higher precidence operators.

        Show
        Tim Hunt added a comment - I think that they should almost never be used. If they are used, there should be whitespace: $row = ($i % 2) ? 'odd' : 'even'; (After all, we require whitespace around +, - etc, and they are higher precidence operators.
        Hide
        Martin Dougiamas added a comment -

        I use the readability criteria. I'd say that in almost every case an if statement is much faster for a brain to parse so let's just say no to them.

        Show
        Martin Dougiamas added a comment - I use the readability criteria. I'd say that in almost every case an if statement is much faster for a brain to parse so let's just say no to them.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Not sure if this was ignited by my fix @ MDL-32786, hehe:

        $service = isset($this->_customdata) ? $this->_customdata : new stdClass();
        

        But in my mind, sentences like that are, are more readable than:

        $service = new stdClass();
        if (isset($this->_customdata)) {
            $service = $this->_customdata;
        }
        
        or:
        
        if (isset($this->_customdata)) {
            $service = $this->_customdata;
        } else {
            $service = new stdClass();
        }
        

        So, for any potential use:

        • simple to evaluate.
        • short to write

        I prefer the ternaries. They guarantee definition and assignment happen and are easily readable (again, in my mind). Of course, with spaces around "?" and ":".

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Not sure if this was ignited by my fix @ MDL-32786 , hehe: $service = isset($ this ->_customdata) ? $ this ->_customdata : new stdClass(); But in my mind, sentences like that are, are more readable than: $service = new stdClass(); if (isset($ this ->_customdata)) { $service = $ this ->_customdata; } or: if (isset($ this ->_customdata)) { $service = $ this ->_customdata; } else { $service = new stdClass(); } So, for any potential use: simple to evaluate. short to write I prefer the ternaries. They guarantee definition and assignment happen and are easily readable (again, in my mind). Of course, with spaces around "?" and ":". Ciao
        Hide
        Michael de Raadt added a comment -

        I'm not sure I would agree with banning them altogether. I agree with Eloy that in many cases the result can be more readable code.

        I would suggest that we suggest that conditional operators be avoided generally and especially where the test is more than a simple value check or the value operands are complex operations. Certainly chained nesting of conditional operators should be avoided.

        Show
        Michael de Raadt added a comment - I'm not sure I would agree with banning them altogether. I agree with Eloy that in many cases the result can be more readable code. I would suggest that we suggest that conditional operators be avoided generally and especially where the test is more than a simple value check or the value operands are complex operations. Certainly chained nesting of conditional operators should be avoided.
        Hide
        Martin Dougiamas added a comment - - edited

        When I look at

         $service = isset($this->_customdata) ? $this->_customdata : new stdClass();
        

        my brain has to slow down and read back and forth a couple of times to make sure I understand it, but for:

        if (isset($this->_customdata)) {
            $service = $this->_customdata;
        } else {
            $service = new stdClass();
        }
        

        the logic is explicit from the structure, and it's much faster for my brain to parse. Code can always be shortened (and to some extent it's a way of showing one's prowess) but if the actual CPU time is the same then I think we need to think about saving brain CPU cycles.

        Since that's the case, it probably affects integrators most, so let's decide this on a vote from all the integrators.

        Vote +1 for allowing them in certain small cases.

        Vote -1 for banning them altogether.

        Show
        Martin Dougiamas added a comment - - edited When I look at $service = isset($ this ->_customdata) ? $ this ->_customdata : new stdClass(); my brain has to slow down and read back and forth a couple of times to make sure I understand it, but for: if (isset($ this ->_customdata)) { $service = $ this ->_customdata; } else { $service = new stdClass(); } the logic is explicit from the structure, and it's much faster for my brain to parse. Code can always be shortened (and to some extent it's a way of showing one's prowess) but if the actual CPU time is the same then I think we need to think about saving brain CPU cycles. Since that's the case, it probably affects integrators most, so let's decide this on a vote from all the integrators. Vote +1 for allowing them in certain small cases. Vote -1 for banning them altogether.
        Hide
        Dan Poltawski added a comment -

        -1 (unless someone can actually define the certain small cases in a sentence that can be enforced)

        Show
        Dan Poltawski added a comment - -1 (unless someone can actually define the certain small cases in a sentence that can be enforced)
        Hide
        Sam Hemelryk added a comment - - edited

        +1 (Changed my vote sorry, thought about this more)

        Show
        Sam Hemelryk added a comment - - edited +1 (Changed my vote sorry, thought about this more)
        Hide
        Martin Dougiamas added a comment -

        Apu and Eloy can you vote?

        Show
        Martin Dougiamas added a comment - Apu and Eloy can you vote?
        Hide
        Eloy Lafuente (stronk7) added a comment - - edited

        +1 (for short ones, those that can be "said" with one English phrase not including any boolean (and / or) word and fitting into one valid (xxx chars) line).

        Show
        Eloy Lafuente (stronk7) added a comment - - edited +1 (for short ones, those that can be "said" with one English phrase not including any boolean (and / or) word and fitting into one valid (xxx chars) line).
        Hide
        Matt Gibson added a comment -

        +1

        For a slightly different case, PHP 5.3 allows this

        $service = !empty($service) ? $service : new stdClass();

        to be rewritten as this

        $service ?: new stdClass();

        So we should also include this option in the guidelines one way or another. I'd say that mentally replacing '?:' with 'or if it's not there, use:' is an improvement to the readability, compared to the long form.

        How about

        $service = $this->_customdata;
        $service ?: new stdClass();

        For cases where there won't be an error when a variable/property has not been initialised, of course.

        http://php.net/manual/en/language.operators.comparison.php

        Show
        Matt Gibson added a comment - +1 For a slightly different case, PHP 5.3 allows this $service = !empty($service) ? $service : new stdClass(); to be rewritten as this $service ?: new stdClass(); So we should also include this option in the guidelines one way or another. I'd say that mentally replacing '?:' with 'or if it's not there, use:' is an improvement to the readability, compared to the long form. How about $service = $ this ->_customdata; $service ?: new stdClass(); For cases where there won't be an error when a variable/property has not been initialised, of course. http://php.net/manual/en/language.operators.comparison.php
        Hide
        Aparup Banerjee added a comment -

        +1 for short sensible ones and variables are self-descriptive or surrounding code lends a hand to the context of the variables.

        i do read it in my mind personally as (if this)?(then this):(else that).

        i think its ok to have the ternary to sort of hint at the level of contextual knowledge needed for a complex code area too. I think thats the main reason for my +1 atm. Its not easy to show this otherwise.

        this however totally breaks the ability to read it as a sentence for me.

        $service ?: new stdClass();
        
        Show
        Aparup Banerjee added a comment - +1 for short sensible ones and variables are self-descriptive or surrounding code lends a hand to the context of the variables. i do read it in my mind personally as (if this)?(then this):(else that). i think its ok to have the ternary to sort of hint at the level of contextual knowledge needed for a complex code area too. I think thats the main reason for my +1 atm. Its not easy to show this otherwise. this however totally breaks the ability to read it as a sentence for me. $service ?: new stdClass();
        Hide
        Dan Poltawski added a comment -
        Show
        Dan Poltawski added a comment - I think we had concencous on this, so I updated the coding style document: http://docs.moodle.org/dev/index.php?title=Coding_style&diff=35426&oldid=34823 http://docs.moodle.org/dev/Coding_style#Ternary_Operator

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development