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

      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;

        Gliffy Diagrams

          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