Moodle
  1. Moodle
  2. MDL-41888

separate out quiz statistics calculations from quiz statistics report

    Details

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

      Description

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

        Issue Links

          Activity

          Hide
          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
          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
          Jamie Pratt added a comment -

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

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

          Great. Submitting for integration.

          Show
          Tim Hunt added a comment - Great. Submitting for integration.
          Hide
          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
          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 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 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
          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
          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
          Tim Hunt added a comment -

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

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

          Rebased this.

          Show
          Jamie Pratt added a comment - Rebased this.
          Hide
          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
          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
          Sam Hemelryk added a comment -

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

          Show
          Sam Hemelryk added a comment - MDL-42055 created to tidy up the phpdocs. (before 2.6 release please)
          Hide
          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
          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
          David Monllaó added a comment -

          Passing as CI unit tests are all passing

          Show
          David Monllaó added a comment - Passing as CI unit tests are all passing
          Hide
          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
          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: