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

          Attachments

            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