Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41888

separate out quiz statistics calculations from quiz statistics report

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.6
    • Component/s: Quiz
    • Labels:

      Description

      I also reworked the code to clarify it and make it more easily maintainable.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            timhunt Tim Hunt added a comment -

            Looks OK.

            My only question is, should you be using the new class auto-loading. That is, instead of quizstatistics.php, should you have two separate file classes/calculator.php and classes/statistics.php ?

            Show
            timhunt Tim Hunt added a comment - Looks OK. My only question is, should you be using the new class auto-loading. That is, instead of quizstatistics.php, should you have two separate file classes/calculator.php and classes/statistics.php ?
            Hide
            jamiesensei Jamie Pratt added a comment -

            I have rebased and am also now using the class autoloading here.

            Show
            jamiesensei Jamie Pratt added a comment - I have rebased and am also now using the class autoloading here.
            Hide
            timhunt Tim Hunt added a comment -

            Great. Submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Great. Submitting for integration.
            Hide
            jamiesensei Jamie Pratt added a comment - - edited

            I added another commit to this branch. I needed to make a small addition to the branch so that my fix for MDL-41929, which inadvertently got fixed in the general clean up done in this issue, is consistent with Tim's fix for earlier stable branches.

            Show
            jamiesensei Jamie Pratt added a comment - - edited I added another commit to this branch. I needed to make a small addition to the branch so that my fix for MDL-41929 , which inadvertently got fixed in the general clean up done in this issue, is consistent with Tim's fix for earlier stable branches.
            Hide
            marina Marina Glancy added a comment -

            Hi Jamie,
            unfortunately your issue will not make it in this integration cycle - very busy week, we have to delay quite a lot of issues. But I quickly looked through the code and would really ask you to look at coding style - double empty lines, absent phpdocs. At least functions should have phpdocs with a description in the beginning. Highly preferable if public properties have /** @var TYPE ... */ too.

            I realise that you just move code from one place to another but please use the chance to improve the readability.

            PS This is not a full review

            Show
            marina Marina Glancy added a comment - Hi Jamie, unfortunately your issue will not make it in this integration cycle - very busy week, we have to delay quite a lot of issues. But I quickly looked through the code and would really ask you to look at coding style - double empty lines, absent phpdocs. At least functions should have phpdocs with a description in the beginning. Highly preferable if public properties have /** @var TYPE ... */ too. I realise that you just move code from one place to another but please use the chance to improve the readability. PS This is not a full review
            Hide
            jamiesensei Jamie Pratt added a comment -

            Hi Marina,

            Thanks for your feedback. I have added more comments to the code and I added a commit (https://github.com/jamiepratt/moodle/commit/e1860f2d126dfe9ddf211a37394f3c6de3932f8d) to a later issue's branch that removes all the extra new lines across the quiz and question statistics code.

            Cheers.

            Show
            jamiesensei Jamie Pratt added a comment - Hi Marina, Thanks for your feedback. I have added more comments to the code and I added a commit ( https://github.com/jamiepratt/moodle/commit/e1860f2d126dfe9ddf211a37394f3c6de3932f8d ) to a later issue's branch that removes all the extra new lines across the quiz and question statistics code. Cheers.
            Hide
            timhunt Tim Hunt added a comment -

            Increasing priority, as discussed with Dan, since this issue blocks others.

            Show
            timhunt Tim Hunt added a comment - Increasing priority, as discussed with Dan, since this issue blocks others.
            Hide
            jamiesensei Jamie Pratt added a comment -

            Rebased this.

            Show
            jamiesensei Jamie Pratt added a comment - Rebased this.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Jamie this has been integrated now.

            The only thing I noted was the phpdoc's as Marina noted as well.
            Rather than delay this issue to tidy them up I'll create a new issue to address the missing phpdocs.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Jamie this has been integrated now. The only thing I noted was the phpdoc's as Marina noted as well. Rather than delay this issue to tidy them up I'll create a new issue to address the missing phpdocs. Many thanks Sam
            Hide
            samhemelryk Sam Hemelryk added a comment -

            MDL-42055 created to tidy up the phpdocs. (before 2.6 release please)

            Show
            samhemelryk Sam Hemelryk added a comment - MDL-42055 created to tidy up the phpdocs. (before 2.6 release please)
            Hide
            timhunt Tim Hunt added a comment -

            I thought Jamie had finished the PHPdoc comments: https://github.com/jamiepratt/moodle/commit/522bef80308581b4cd9835cdd94219f444c35ea0. I think MDL-42055 can be closed. Either that, or you should add more specific details there.

            Show
            timhunt Tim Hunt added a comment - I thought Jamie had finished the PHPdoc comments: https://github.com/jamiepratt/moodle/commit/522bef80308581b4cd9835cdd94219f444c35ea0 . I think MDL-42055 can be closed. Either that, or you should add more specific details there.
            Hide
            dmonllao David Monllaó added a comment -

            Passing as CI unit tests are all passing

            Show
            dmonllao David Monllaó added a comment - Passing as CI unit tests are all passing
            Hide
            poltawski Dan Poltawski added a comment -

            You did it!

            Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

            Show
            poltawski Dan Poltawski added a comment - You did it! Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  18/Nov/13