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

Coding style: Allow no phpdoc for methods that override base class

    Details

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

      Description

      For methods that override base class, e.g. moodle_filter::filter method, it is unclear what to do regarding phpdoc.

      Because the function documentation block is basically supposed to describe the 'contract' of the function (parameters, returns) and not necessarily the detail of what goes on inside it (that would be done with inline comments), and because that contract is defined by the base class method, in most cases the overriding method does not need different documentation from the base class method.

      In Java best practice for this (imo) is to simply not document the method (except for preferably using the @Override annotation). The IDE will automatically inherit correct documentation from the base class.

      PHP does not have the @Override annotation so there are a number of options:

      1. Copy/paste the documentation from the base class, so that it duplicates the 'real' documentation and gradually becomes out of date or poorer quality as time passes

      OPINION: This is very very bad, and it is also what the current coding guidelines suggest if followed literally.

      2. Include a doc block like this:

      /**
       * @see moodle_text_filter::filter()
       */
      

      OPINION: This is not too bad (no duplication) but it is ugly.

      3. Omit doc block entirely for functions which are defined in base class.

      OPINION: This is the best way to proceed.

        Activity

        Hide
        Dan Poltawski added a comment -

        Pasting in some discussion about this (hope Eloy doesn't mind!)

        15:31:01 stronk7: about phpdocs of overriden methods I'm divided. For me it's clear that both interface and abstract implementations don't need phpdocs.

        But truly overridden methods... if they have been overridden there must be a reason and it should be explained somewhere.

        Perhaps we should make the phpdocs checker to stop claiming about missing docs and only analyze the existing docs instead. Else we would need to build the whole class-map of Moodle to be able to discern.
        15:34:31 aparup apu: well one way is to only take it as feedback.. then this inaccuracy is ok. but if it ever becomes the definition of yes/no then it should stop the inaccurate bits
        15:36:24 stronk7: yes, yes, i docs are present they must be valid always.
        15:36:28 stronk7: *if

        Show
        Dan Poltawski added a comment - Pasting in some discussion about this (hope Eloy doesn't mind!) 15:31:01 stronk7: about phpdocs of overriden methods I'm divided. For me it's clear that both interface and abstract implementations don't need phpdocs. But truly overridden methods... if they have been overridden there must be a reason and it should be explained somewhere. Perhaps we should make the phpdocs checker to stop claiming about missing docs and only analyze the existing docs instead. Else we would need to build the whole class-map of Moodle to be able to discern. 15:34:31 aparup apu: well one way is to only take it as feedback.. then this inaccuracy is ok. but if it ever becomes the definition of yes/no then it should stop the inaccurate bits 15:36:24 stronk7: yes, yes, i docs are present they must be valid always. 15:36:28 stronk7: *if
        Hide
        Dan Poltawski added a comment -

        Adding integrators here.

        The advantage of Sam M's option 2 is that it means the developer explicitly has to define 'there is nothing to document here' (sortof).

        Show
        Dan Poltawski added a comment - Adding integrators here. The advantage of Sam M's option 2 is that it means the developer explicitly has to define 'there is nothing to document here' (sortof).
        Hide
        Sam Marshall added a comment -

        Re my option 2, it is actually less ugly if we decide it should be on one line:

        /** @see moodle_text_filter::filter() */
        

        I wonder if this @see thing actually works OK in other phpdoc tools?

        Re eloy's point about 'true' override sometimes needing documentation I agree that can sometimes happen but in my experience not very often. Especially, it's often the case that base class has a default implementation that does nothing or whatever - so it isn't literally only abstract/interfaces that this affects.

        IMO If it still achieves the same purpose it doesn't need a new doc, you only need the doc if you are changing the purpose somehow or introducing some side effects (like bad performance) or something. For example, let's suppose there is a base class get_title() function which normally calls get_string using the class name or something. If you override this in your implementation to get the title in some other way (maybe it includes some user config data in the title) you do not need to change the doc because the function still does the same thing. HOW you go about getting the title is kind of up to you (& should be documented with inline comments inside the method).

        Show
        Sam Marshall added a comment - Re my option 2, it is actually less ugly if we decide it should be on one line: /** @see moodle_text_filter::filter() */ I wonder if this @see thing actually works OK in other phpdoc tools? Re eloy's point about 'true' override sometimes needing documentation I agree that can sometimes happen but in my experience not very often. Especially, it's often the case that base class has a default implementation that does nothing or whatever - so it isn't literally only abstract/interfaces that this affects. IMO If it still achieves the same purpose it doesn't need a new doc, you only need the doc if you are changing the purpose somehow or introducing some side effects (like bad performance) or something. For example, let's suppose there is a base class get_title() function which normally calls get_string using the class name or something. If you override this in your implementation to get the title in some other way (maybe it includes some user config data in the title) you do not need to change the doc because the function still does the same thing. HOW you go about getting the title is kind of up to you (& should be documented with inline comments inside the method).
        Hide
        Aparup Banerjee added a comment -

        http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.inlineinheritdoc.pkg.html seems to apply here. Alas its only for classes.

        i think its ok to either

        • duplicate and customize the phpdoc for the overiding method.
          or
        • have no phpdoc at all.

        anything else will break phpdoc parsing features it seems. (see cool auto completion feature)

        Show
        Aparup Banerjee added a comment - http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.inlineinheritdoc.pkg.html seems to apply here. Alas its only for classes. i think its ok to either duplicate and customize the phpdoc for the overiding method. or have no phpdoc at all. anything else will break phpdoc parsing features it seems. ( see cool auto completion feature )
        Hide
        Dan Poltawski added a comment -
        Show
        Dan Poltawski added a comment - Interesting blog post from the guys at liip http://blog.liip.ch/archive/2011/07/26/phpdoc-compilers-and-inheritdoc.html
        Hide
        Sam Marshall added a comment -

        note: @see is listed in moodle coding guidelines to it should be OK to use (pending a decision on this issue).

        Show
        Sam Marshall added a comment - note: @see is listed in moodle coding guidelines to it should be OK to use (pending a decision on this issue).
        Hide
        Marina Glancy added a comment -

        I vote for @see or just plain comment that says "Overwrites parent function" or "Overwrites

        {@link parentclass::function_name()}

        "
        Sometimes it still needs additional comment about what's going on in this particular implementation

        Show
        Marina Glancy added a comment - I vote for @see or just plain comment that says "Overwrites parent function" or "Overwrites {@link parentclass::function_name()} " Sometimes it still needs additional comment about what's going on in this particular implementation
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I vote for 3) aka, accept (without forbiding) overridden implementations without phpdocs nor hints at all.

        IDES and Systems X do know how to proceed (inherit doc). And IDEs not supporting that are just 1-2 clicks away from the documentation (worst case).

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - I vote for 3) aka, accept (without forbiding) overridden implementations without phpdocs nor hints at all. IDES and Systems X do know how to proceed (inherit doc). And IDEs not supporting that are just 1-2 clicks away from the documentation (worst case). Ciao
        Hide
        Damyon Wiese added a comment -

        One issue with 3, is that if you haven't changed anything about the function, why did you override it ?

        For overridden methods I would just expect at a minimum a one line description of whats different from the parent.

        For example:
        https://github.com/sammarshallou/moodle/compare/master...MDL-41253-master#L3R75

        Show
        Damyon Wiese added a comment - One issue with 3, is that if you haven't changed anything about the function, why did you override it ? For overridden methods I would just expect at a minimum a one line description of whats different from the parent. For example: https://github.com/sammarshallou/moodle/compare/master...MDL-41253-master#L3R75
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Like, "this is insert_record() for mysql", "this is insert_record() for oracle" ?

        Just saying that, if anything needs to be documented, np, at all, the (complete) phpdoc block is added and perfect.

        But that does not happen always and there are cases where there are nothing "special" to document, hence make it optional (and auto-inherited if missing).

        That's my 3). Make it optional.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Like, "this is insert_record() for mysql", "this is insert_record() for oracle" ? Just saying that, if anything needs to be documented, np, at all, the (complete) phpdoc block is added and perfect. But that does not happen always and there are cases where there are nothing "special" to document, hence make it optional (and auto-inherited if missing). That's my 3). Make it optional. Ciao
        Hide
        Sam Marshall added a comment -

        Damyon - Eloy kind of said it, but just to make it explicit, the short answer to your question is: abstract or interface methods that you are required to override but which are fully documented in parent (or those where the parent has a null/minimal implementation but are otherwise the same way).

        As an example that isn't literally abstract but is pretty much like it: whenever you override the validation form method, your method is obviously going to be validating the form, there is no need to document that any more.

        Even in case where you are changing behaviour for a subclass it is also arguable that, because the method comment is supposed to document what the method does overall (a sort of black-box view), it may well be more appropriate to document any changes specific to your subclass as inline comments inside the method rather than in the method docblock, if they don't affect the caller's view of the method.

        So there is a possibility of varying levels of optional-ness...

        Show
        Sam Marshall added a comment - Damyon - Eloy kind of said it, but just to make it explicit, the short answer to your question is: abstract or interface methods that you are required to override but which are fully documented in parent (or those where the parent has a null/minimal implementation but are otherwise the same way). As an example that isn't literally abstract but is pretty much like it: whenever you override the validation form method, your method is obviously going to be validating the form, there is no need to document that any more. Even in case where you are changing behaviour for a subclass it is also arguable that, because the method comment is supposed to document what the method does overall (a sort of black-box view), it may well be more appropriate to document any changes specific to your subclass as inline comments inside the method rather than in the method docblock, if they don't affect the caller's view of the method. So there is a possibility of varying levels of optional-ness...
        Hide
        Damyon Wiese added a comment -

        Thanks - optional is fine (But I foresee optional == parent has docs so I don't need to think about them ever).

        Show
        Damyon Wiese added a comment - Thanks - optional is fine (But I foresee optional == parent has docs so I don't need to think about them ever).
        Hide
        Marina Glancy added a comment -

        while searching for solutions found an interesting tag @inheritdoc - includes the parent class documentation in child class. Does not say anything about methods though
        http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.inlineinheritdoc.pkg.html

        Show
        Marina Glancy added a comment - while searching for solutions found an interesting tag @inheritdoc - includes the parent class documentation in child class. Does not say anything about methods though http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.inlineinheritdoc.pkg.html
        Hide
        Aparup Banerjee added a comment -

        whatever happened to phpdoc system X? that would have to be inline with tag support for use here.

        Show
        Aparup Banerjee added a comment - whatever happened to phpdoc system X? that would have to be inline with tag support for use here.
        Hide
        Frédéric Massart added a comment -

        I personally like to see the documentation along with the method, whether it is overriding something or not. It is much easier to find that inline rather than having to jump somewhere else, read, jump back and then check another method called in that method, jump again, jump back, wait one more method call... jump again to the parent, etc... I don't think the documentation would be outdated if updated whenever the overriding method is updated.

        If we adopt the option to not write anything at all, then it becomes really annoying to figure out if the method is overriding one of the parents or not.

        Of course IDEs can help a lot to find the parent method, though when you have a cascading of overridden classes (say in backup), it will be a pain to go back to the first declaration of the method, read, and then read the code logic, etc...

        Show
        Frédéric Massart added a comment - I personally like to see the documentation along with the method, whether it is overriding something or not. It is much easier to find that inline rather than having to jump somewhere else, read, jump back and then check another method called in that method, jump again, jump back, wait one more method call... jump again to the parent, etc... I don't think the documentation would be outdated if updated whenever the overriding method is updated. If we adopt the option to not write anything at all , then it becomes really annoying to figure out if the method is overriding one of the parents or not. Of course IDEs can help a lot to find the parent method, though when you have a cascading of overridden classes (say in backup), it will be a pain to go back to the first declaration of the method, read, and then read the code logic, etc...
        Hide
        Ankit Agarwal added a comment - - edited

        Just realised, my editor(phpstorm) completely ignores the overridden phpdoc block and shows parent dock blocks for overridden methods , when trying to see the doc inline.

        {@inhertdoc}

        doesn't seem to make any difference for methods.

        Having said that, it is always nice to have doc along with the api.

        Show
        Ankit Agarwal added a comment - - edited Just realised, my editor(phpstorm) completely ignores the overridden phpdoc block and shows parent dock blocks for overridden methods , when trying to see the doc inline. {@inhertdoc} doesn't seem to make any difference for methods. Having said that, it is always nice to have doc along with the api.
        Hide
        Eloy Lafuente (stronk7) added a comment - - edited

        FYI, after agreement this has been decided about overridden methods:

        1) no documentation IS allowed.
        2) full documentation IS allowed.
        3) @inheritdoc and @see are NOT allowed.

        So closing this and going to reflect the changes @ http://docs.moodle.org/dev/Coding_style#PHPDoc

        Thanks all, ciao

        Edited: grrr, s/overloaded/overridden

        Show
        Eloy Lafuente (stronk7) added a comment - - edited FYI, after agreement this has been decided about overridden methods: 1) no documentation IS allowed. 2) full documentation IS allowed. 3) @inheritdoc and @see are NOT allowed. So closing this and going to reflect the changes @ http://docs.moodle.org/dev/Coding_style#PHPDoc Thanks all, ciao Edited: grrr, s/overloaded/overridden

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development