Uploaded image for project: 'Moodle Community Sites'
  1. Moodle Community Sites
  2. MDLSITE-1766

We need coding style guidance for the ternary operator

    Details

    • Type: Task
    • Status: Closed
    • Priority: 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

          Attachments

            Activity

            Hide
            poltawski 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
            poltawski 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
            samhemelryk 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
            samhemelryk 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
            timhunt 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
            timhunt 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
            dougiamas 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
            dougiamas 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
            stronk7 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
            stronk7 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
            salvetore 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
            salvetore 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
            dougiamas 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
            dougiamas 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
            poltawski Dan Poltawski added a comment -

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

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

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

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

            Apu and Eloy can you vote?

            Show
            dougiamas Martin Dougiamas added a comment - Apu and Eloy can you vote?
            Hide
            stronk7 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
            stronk7 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
            mattgibson 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
            mattgibson 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
            nebgor 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
            nebgor 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
            poltawski Dan Poltawski added a comment -
            Show
            poltawski 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: