From bccad386b0ff0dacfc6c4e8712f73a1ac8fd60fd Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 20 Dec 2017 17:55:57 +0000 Subject: [PATCH] MDL-61077 question stats: make calculations more robust --- mod/quiz/report/statistics/report.php | 2 +- .../all_calculated_for_qubaid_condition.php | 48 ++++++++++++++++--- .../statistics/questions/calculator.php | 6 +-- .../responses/analysis_for_question.php | 5 ++ .../responses/analysis_for_subpart.php | 17 +++++-- 5 files changed, 62 insertions(+), 16 deletions(-) diff --git a/mod/quiz/report/statistics/report.php b/mod/quiz/report/statistics/report.php index 790be61d708..84cfe2720f7 100644 --- a/mod/quiz/report/statistics/report.php +++ b/mod/quiz/report/statistics/report.php @@ -201,7 +201,7 @@ class quiz_statistics_report extends quiz_default_report { } else if ($qid) { // Report on an individual sub-question indexed questionid. - if (is_null($questionstats->for_subq($qid, $variantno))) { + if (!$questionstats->has_subq($qid, $variantno)) { print_error('questiondoesnotexist', 'question'); } diff --git a/question/classes/statistics/questions/all_calculated_for_qubaid_condition.php b/question/classes/statistics/questions/all_calculated_for_qubaid_condition.php index bafebfe3bd3..592c12642e4 100644 --- a/question/classes/statistics/questions/all_calculated_for_qubaid_condition.php +++ b/question/classes/statistics/questions/all_calculated_for_qubaid_condition.php @@ -95,23 +95,41 @@ class all_calculated_for_qubaid_condition { } } + /** + * Do we have stats for a particular quesitonid (and optionally variant)? + * + * @param int $questionid The id of the sub question. + * @param int|null $variant if not null then we want the object to store a variant of a sub-question's stats. + * @return bool whether those stats exist (yet). + */ + public function has_subq($questionid, $variant = null) { + if ($variant === null) { + return isset($this->subquestionstats[$questionid]); + } else { + return isset($this->subquestionstats[$questionid]->variantstats[$variant]); + } + } + /** * Reference for a item stats instance for a questionid and optional variant no. * * @param int $questionid The id of the sub question. * @param int|null $variant if not null then we want the object to store a variant of a sub-question's stats. - * @return calculated_for_subquestion|null null if the stats object does not yet exist. + * @return calculated|calculated_for_subquestion stats instance for a questionid and optional variant no. + * Will be a calculated_for_subquestion if no variant specified. + * @throws \coding_exception if there is an attempt to respond to a non-existant set of stats. */ public function for_subq($questionid, $variant = null) { if ($variant === null) { if (!isset($this->subquestionstats[$questionid])) { - return null; + throw new \coding_exception('Reference to unknown question id ' . $questionid); } else { return $this->subquestionstats[$questionid]; } } else { if (!isset($this->subquestionstats[$questionid]->variantstats[$variant])) { - return null; + throw new \coding_exception('Reference to unknown question id ' . $questionid . + ' variant ' . $variant); } else { return $this->subquestionstats[$questionid]->variantstats[$variant]; } @@ -136,23 +154,39 @@ class all_calculated_for_qubaid_condition { return array_keys($this->questionstats); } + /** + * Do we have stats for a particular slot (and optionally variant)? + * + * @param int $slot The slot no. + * @param int|null $variant if provided then we want the object which stores a variant of a position's stats. + * @return bool whether those stats exist (yet). + */ + public function has_slot($slot, $variant = null) { + if ($variant === null) { + return isset($this->questionstats[$slot]); + } else { + return isset($this->questionstats[$slot]->variantstats[$variant]); + } + } + /** * Get position stats instance for a slot and optional variant no. * * @param int $slot The slot no. - * @param null $variant if provided then we want the object which stores a variant of a position's stats. - * @return calculated|null An instance of the class storing the calculated position stats. + * @param int|null $variant if provided then we want the object which stores a variant of a position's stats. + * @return calculated|calculated_for_subquestion An instance of the class storing the calculated position stats. + * @throws \coding_exception if there is an attempt to respond to a non-existant set of stats. */ public function for_slot($slot, $variant = null) { if ($variant === null) { if (!isset($this->questionstats[$slot])) { - return null; + throw new \coding_exception('Reference to unknown slot ' . $slot); } else { return $this->questionstats[$slot]; } } else { if (!isset($this->questionstats[$slot]->variantstats[$variant])) { - return null; + throw new \coding_exception('Reference to unknown slot ' . $slot . ' variant ' . $variant); } else { return $this->questionstats[$slot]->variantstats[$variant]; } diff --git a/question/classes/statistics/questions/calculator.php b/question/classes/statistics/questions/calculator.php index 4f0f2853fcb..66249122a3b 100644 --- a/question/classes/statistics/questions/calculator.php +++ b/question/classes/statistics/questions/calculator.php @@ -106,7 +106,7 @@ class calculator { $israndomquestion = ($step->questionid != $this->stats->for_slot($step->slot)->questionid); $breakdownvariants = !$israndomquestion && $this->stats->for_slot($step->slot)->break_down_by_variant(); // If this is a variant we have not seen before create a place to store stats calculations for this variant. - if ($breakdownvariants && is_null($this->stats->for_slot($step->slot , $step->variant))) { + if ($breakdownvariants && !$this->stats->has_slot($step->slot, $step->variant)) { $question = $this->stats->for_slot($step->slot)->question; $this->stats->initialise_for_slot($step->slot, $question, $step->variant); $this->stats->for_slot($step->slot, $step->variant)->randomguessscore = @@ -118,14 +118,14 @@ class calculator { // If this is a random question do the calculations for sub question stats. if ($israndomquestion) { - if (is_null($this->stats->for_subq($step->questionid))) { + if (!$this->stats->has_subq($step->questionid)) { $this->stats->initialise_for_subq($step); } else if ($this->stats->for_subq($step->questionid)->maxmark != $step->maxmark) { $this->stats->for_subq($step->questionid)->differentweights = true; } // If this is a variant of this subq we have not seen before create a place to store stats calculations for it. - if (is_null($this->stats->for_subq($step->questionid, $step->variant))) { + if (!$this->stats->has_subq($step->questionid, $step->variant)) { $this->stats->initialise_for_subq($step, $step->variant); } diff --git a/question/classes/statistics/responses/analysis_for_question.php b/question/classes/statistics/responses/analysis_for_question.php index 5be3e4213f8..3e06f3cf171 100644 --- a/question/classes/statistics/responses/analysis_for_question.php +++ b/question/classes/statistics/responses/analysis_for_question.php @@ -128,6 +128,11 @@ class analysis_for_question { if (!isset($this->subparts[$variantno])) { $this->initialise_stats_for_variant($variantno); } + if (!isset($this->subparts[$variantno][$subpartid])) { + debugging('Unexpected sub-part id ' . $subpartid . + ' encountered.'); + $this->subparts[$variantno][$subpartid] = new analysis_for_subpart(); + } return $this->subparts[$variantno][$subpartid]; } diff --git a/question/classes/statistics/responses/analysis_for_subpart.php b/question/classes/statistics/responses/analysis_for_subpart.php index a9e8a868067..cbb7352485a 100644 --- a/question/classes/statistics/responses/analysis_for_subpart.php +++ b/question/classes/statistics/responses/analysis_for_subpart.php @@ -47,6 +47,11 @@ namespace core_question\statistics\responses; */ class analysis_for_subpart { + /** + * @var analysis_for_class[] + */ + protected $responseclasses; + /** * Takes an array of possible_responses as returned from {@link \question_type::get_possible_responses()}. * @@ -57,14 +62,11 @@ class analysis_for_subpart { foreach ($responseclasses as $responseclassid => $responseclass) { $this->responseclasses[$responseclassid] = new analysis_for_class($responseclass, $responseclassid); } + } else { + $this->responseclasses = []; } } - /** - * @var analysis_for_class[] - */ - protected $responseclasses; - /** * Unique ids for response classes. * @@ -81,7 +83,12 @@ class analysis_for_subpart { * @return analysis_for_class */ public function get_response_class($classid) { + if (!isset($this->responseclasses[$classid])) { + debugging('Unexpected class id ' . $classid . ' encountered.'); + $this->responseclasses[$classid] = new analysis_for_class('[Unknown]', $classid); + } return $this->responseclasses[$classid]; + } /**