Moodle
  1. Moodle
  2. MDL-32322

Code cleanup requried in the quiz reports

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.5, 2.2.2
    • Fix Version/s: 2.3
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      Testing instructions:

      Well, really, we need a full round of QA testing on the quiz reports. Fortunately, we have one of those coming up.

      I would say, don't spend too much time testing this now, because I am about to make a lot more changes for MDL-3030. Let us wait for those changes to go in, then test the lot.

      On the other hand, it would be worth doing some quiz testing now.

      Find (or create) a course with

      • groups
      • a quiz with attempts by various students
      • who are in different groups
      • ensure the quiz is set to separate groups mode.

      Then, look at each report, for All students, and some groups, and for various options, and make sure nothing is obviously broken.

      Show
      Testing instructions: Well, really, we need a full round of QA testing on the quiz reports. Fortunately, we have one of those coming up. I would say, don't spend too much time testing this now, because I am about to make a lot more changes for MDL-3030 . Let us wait for those changes to go in, then test the lot. On the other hand, it would be worth doing some quiz testing now. Find (or create) a course with groups a quiz with attempts by various students who are in different groups ensure the quiz is set to separate groups mode. Then, look at each report, for All students, and some groups, and for various options, and make sure nothing is obviously broken.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      39131

      Description

      Things got a lot better in Moodle 2.1, but there is still a lot of duplication between mod/quiz/report/overview and mod/quiz/report/responses.

      I just got into a terrible mess trying to clean this up as part of MDL-3030, so now I am making a separate branch, so I can clean this up separately.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          I have decided to submit this for integration now.

          The code could still be improved further, for example there is still a lot of duplication that could be factored out, and it would be really good to extract all the output code into renderers.

          However, the code is now in a state where I can finish implementing MDL-3030 without having to edit code in two places, and, the priority right now is to finish MDL-3030 before the 2.3 code freeze.

          Therefore, I will file a new bug for further improvements later, and submit this now. Peer review comments before the end of the week would be very welcome.

          Show
          Tim Hunt added a comment - I have decided to submit this for integration now. The code could still be improved further, for example there is still a lot of duplication that could be factored out, and it would be really good to extract all the output code into renderers. However, the code is now in a state where I can finish implementing MDL-3030 without having to edit code in two places, and, the priority right now is to finish MDL-3030 before the 2.3 code freeze. Therefore, I will file a new bug for further improvements later, and submit this now. Peer review comments before the end of the week would be very welcome.
          Hide
          Tim Hunt added a comment -

          MDL-32478 is the follow-on issue.

          Show
          Tim Hunt added a comment - MDL-32478 is the follow-on issue.
          Hide
          Dan Poltawski added a comment -

          Hi Tim,

          Here is the output from the pre checker - some missing phpdoc picked up

          Show
          Dan Poltawski added a comment - Hi Tim, Here is the output from the pre checker - some missing phpdoc picked up
          Hide
          Dan Poltawski added a comment -

          Hi Tim,

          Looks good to me.

          While reviewing this diff it made me wonder if it really was worth increasing the strictness of the code checker to warn about capital letter and punctation since its produced (imo) unnecessary diff.

          Only thing to note is that now that the unsigned ints have been removed in master I suppose your note (and the SQL) could be reviewed in the future:

          // To explain that last bit, in MySQL, qa.timestart and qa.timefinish
          // are unsigned. Since MySQL 5.5.5, when they introduced strict mode,
          // subtracting a larger unsigned int from a smaller one gave an error.
          // Therefore, we avoid doing that. timefinish can be non-zero and less
          // than timestart when you have two load-balanced servers with very
          // badly synchronised clocks, and a student does a really quick attempt.
          Show
          Dan Poltawski added a comment - Hi Tim, Looks good to me. While reviewing this diff it made me wonder if it really was worth increasing the strictness of the code checker to warn about capital letter and punctation since its produced (imo) unnecessary diff. Only thing to note is that now that the unsigned ints have been removed in master I suppose your note (and the SQL) could be reviewed in the future: // To explain that last bit, in MySQL, qa.timestart and qa.timefinish // are unsigned. Since MySQL 5.5.5, when they introduced strict mode, // subtracting a larger unsigned int from a smaller one gave an error. // Therefore, we avoid doing that. timefinish can be non-zero and less // than timestart when you have two load-balanced servers with very // badly synchronised clocks, and a student does a really quick attempt.
          Hide
          Tim Hunt added a comment - - edited

          Thank you for your review dan.

          1. Regarding the Smurf output, many of the 'missing' PHP doc comments, seem to be about methods in subclasses that override the same method in the base class, and where the method in the base class is properly documented.

          In this situation, I believe it is incorrect to add a comment to the method in the subclass. The PHPdoc comment is meant to document the API contract that the method implements. That is, how someone can use the method as a black box. Therefore, the PHPdoc comment for the method in the subclass should be exactly the comment for the method in the base class.

          If the sub-class method does not have its own PHPdoc comment, then the PHPdoc system will use the base-class comment. I could copy and paste the other comment, but that is just code duplication, and is therefore bad.

          So, really, please can we fix Smurf to not report bogus things. Now, if we wanted to introduce a new bit of coding style rule, something like:

          When overriding a method in a sub-class, you should not need to add a PHPdoc comment. The overridden method should fulfil exactly the same contract as the method in the base class. Instead, add a normal comment like:

          /* Override of {@link base_class_name::this_method_name()}. */

          But that would be a new rule, and if and when you introduce it, I will start following it.

          2. I think, when reviewing patches, it is quite easy to ignore the changes in comments.

          With coding style things, and old code that does not currently follow it, there is always a trade-off between fixing the mess, and keeping the diff simple. I had already taken a big step in the right direction by separating these changes from MDL-3030.

          3. I had already spotted that bit, but only after I had submitted this bug, and gone back to MDL-3030, so I did not change it then. This will probably have to wait until later.

          I will fix the few real PHPdoc errors, rebase, then submit for integration.

          Show
          Tim Hunt added a comment - - edited Thank you for your review dan. 1. Regarding the Smurf output, many of the 'missing' PHP doc comments, seem to be about methods in subclasses that override the same method in the base class, and where the method in the base class is properly documented. In this situation, I believe it is incorrect to add a comment to the method in the subclass. The PHPdoc comment is meant to document the API contract that the method implements. That is, how someone can use the method as a black box. Therefore, the PHPdoc comment for the method in the subclass should be exactly the comment for the method in the base class. If the sub-class method does not have its own PHPdoc comment, then the PHPdoc system will use the base-class comment. I could copy and paste the other comment, but that is just code duplication, and is therefore bad. So, really, please can we fix Smurf to not report bogus things. Now, if we wanted to introduce a new bit of coding style rule, something like: When overriding a method in a sub-class, you should not need to add a PHPdoc comment. The overridden method should fulfil exactly the same contract as the method in the base class. Instead, add a normal comment like: /* Override of {@link base_class_name::this_method_name()}. */ But that would be a new rule, and if and when you introduce it, I will start following it. 2. I think, when reviewing patches, it is quite easy to ignore the changes in comments. With coding style things, and old code that does not currently follow it, there is always a trade-off between fixing the mess, and keeping the diff simple. I had already taken a big step in the right direction by separating these changes from MDL-3030 . 3. I had already spotted that bit, but only after I had submitted this bug, and gone back to MDL-3030 , so I did not change it then. This will probably have to wait until later. I will fix the few real PHPdoc errors, rebase, then submit for integration.
          Hide
          Dan Poltawski added a comment -
          1. Hmm, actually I am sure I had seen the smurf check do something more intelligent about overridden methods before.
          2. I wasn't saying you shouldn't have fixed it or anything like that. The comment was actually that it adds a lot of 'noise' to the signal to noise ratio of changes in general. In particularly I don't think those comments without punctuation were a mess that needed to be cleaned up.
          Show
          Dan Poltawski added a comment - Hmm, actually I am sure I had seen the smurf check do something more intelligent about overridden methods before. I wasn't saying you shouldn't have fixed it or anything like that. The comment was actually that it adds a lot of 'noise' to the signal to noise ratio of changes in general. In particularly I don't think those comments without punctuation were a mess that needed to be cleaned up.
          Hide
          Tim Hunt added a comment -

          Branch rebased and PHPdoc comments fixed. (Or, as Dan might say, more noise added )

          Submitting for integration now.

          Show
          Tim Hunt added a comment - Branch rebased and PHPdoc comments fixed. (Or, as Dan might say, more noise added ) Submitting for integration now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Curiosity:

          I got merge conflict, because simpletests have been recently deleted, np. The curious thing is that I got the message:

          CONFLICT (modify/delete): mod/quiz/report/statistics/simpletest/test_qstats.php deleted in HEAD and modified in 9d58dae33b5e71943cf977bcc549426732b89d20. Version 9d58dae33b5e71943cf977bcc549426732b89d20 of mod/quiz/report/statistics/simpletest/test_qstats.php left in tree.

          CONFLICT (modify/delete): mod/quiz/report/simpletest/testreportlib.php deleted in HEAD and modified in 9d58dae33b5e71943cf977bcc549426732b89d20. Version 9d58dae33b5e71943cf977bcc549426732b89d20 of mod/quiz/report/simpletest/testreportlib.php left in tree.

          So it seems that those files were modified by 9d58dae33b5e71943cf977bcc549426732b89d20. But that message is misleading because that commit was not the one performing the changes, but just the tip/HEAD of your branch.

          Anyway. I've confirmed the deletion of those 2 files and then added extra commit mimicking the changes in phpunit tests. Also executed them and got:

          OK (1158 tests, 21004 assertions)
          

          So I think it's ok to be sent to integration.git. Doing... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Curiosity: I got merge conflict, because simpletests have been recently deleted, np. The curious thing is that I got the message: CONFLICT (modify/delete): mod/quiz/report/statistics/simpletest/test_qstats.php deleted in HEAD and modified in 9d58dae33b5e71943cf977bcc549426732b89d20. Version 9d58dae33b5e71943cf977bcc549426732b89d20 of mod/quiz/report/statistics/simpletest/test_qstats.php left in tree. CONFLICT (modify/delete): mod/quiz/report/simpletest/testreportlib.php deleted in HEAD and modified in 9d58dae33b5e71943cf977bcc549426732b89d20. Version 9d58dae33b5e71943cf977bcc549426732b89d20 of mod/quiz/report/simpletest/testreportlib.php left in tree. So it seems that those files were modified by 9d58dae33b5e71943cf977bcc549426732b89d20. But that message is misleading because that commit was not the one performing the changes, but just the tip/HEAD of your branch. Anyway. I've confirmed the deletion of those 2 files and then added extra commit mimicking the changes in phpunit tests. Also executed them and got: OK (1158 tests, 21004 assertions) So I think it's ok to be sent to integration.git. Doing... ciao
          Hide
          Tim Hunt added a comment -

          Thanks for sorting that out Eloy.

          Show
          Tim Hunt added a comment - Thanks for sorting that out Eloy.
          Hide
          Rajesh Taneja added a comment -

          Passing the test, as nothing seems to be broken. All reports for group or individual is accessable.

          Thanks Tim

          FYI:
          E_STRICT notice:

          Strict standards: Creating default object from empty value in /var/www/im/mod/quiz/report/statistics/responseanalysis.php on line 185 Call Stack: 0.0002 691784 1. {main}() /var/www/im/mod/quiz/report.php:0 0.2972 60612336 2. quiz_statistics_report->display() /var/www/im/mod/quiz/report.php:100 0.5195 68409248 3. quiz_statistics_report->output_individual_question_response_analysis() /var/www/im/mod/quiz/report/statistics/report.php:211 0.6326 68597024 4. quiz_statistics_response_analyser->load_cached() /var/www/im/mod/quiz/report/statistics/report.php:392 
          
          Show
          Rajesh Taneja added a comment - Passing the test, as nothing seems to be broken. All reports for group or individual is accessable. Thanks Tim FYI: E_STRICT notice: Strict standards: Creating default object from empty value in / var /www/im/mod/quiz/report/statistics/responseanalysis.php on line 185 Call Stack: 0.0002 691784 1. {main}() / var /www/im/mod/quiz/report.php:0 0.2972 60612336 2. quiz_statistics_report->display() / var /www/im/mod/quiz/report.php:100 0.5195 68409248 3. quiz_statistics_report->output_individual_question_response_analysis() / var /www/im/mod/quiz/report/statistics/report.php:211 0.6326 68597024 4. quiz_statistics_response_analyser->load_cached() / var /www/im/mod/quiz/report/statistics/report.php:392
          Hide
          Tim Hunt added a comment -

          Thanks Rajesh. I will add the fix for that strict syntax problem to MDL-3030.

          Show
          Tim Hunt added a comment - Thanks Rajesh. I will add the fix for that strict syntax problem to MDL-3030 .
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been near becoming rejected, because it's not the best code you are able to produce.

          But, luckily, at the end, it has landed and has been spread to all repos out there.

          Many thanks and, don't forget it, keep improving your skills, you can!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: