Skip to content

Commit

Permalink
Refactoring:
Browse files Browse the repository at this point in the history 
- Use constants from tables class only.
- Change some subqueries to joins.
- Static variables for verbal feedback instances, when fetching by category or criterion.
  • Loading branch information
@srobotta
srobotta committed Oct 14, 2024
1 parent dcde1b1 commit 4dec38c
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 86 deletions.
122 changes: 53 additions & 69 deletions classes/api.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,14 @@
use dml_exception;
use Exception;
use moodle_exception;
use stdClass;
use core_date;
use mod_verbalfeedback\api\gradebook;
use mod_verbalfeedback\model\response;
use mod_verbalfeedback\model\submission;
use mod_verbalfeedback\model\submission_status;
use mod_verbalfeedback\repository\instance_repository;
use mod_verbalfeedback\repository\submission_repository;
use mod_verbalfeedback\repository\tables;
use mod_verbalfeedback\utils\user;
use mod_verbalfeedback\utils\instance;
use mod_verbalfeedback\utils\user_utils;

// Quick hack for issue #43.
ini_set('memory_limit', '256M');

/**
* Class for performing DB actions for the verbal feedback activity module.
Expand All @@ -53,13 +46,6 @@
*/
class api {

/** The instance database table. */
const DB_INSTANCE = "verbalfeedback";
/** The response database table. */
const DB_RESPONSE = "verbalfeedback_response";
/** The submission database table. */
const DB_SUBMISSION = "verbalfeedback_submission";

/** Status when a user has not yet provided feedback to another user. */
const STATUS_PENDING = 0;
/** Status when a user has begun providing feedback to another user. */
Expand Down Expand Up @@ -101,8 +87,8 @@ public static function get_instance($verbalfeedbackid) {
global $DB;

$id = (int)$verbalfeedbackid;
if (!array_key_exists($id, static::$instances)) {
static::$instances[$id] = $DB->get_record('verbalfeedback', ['id' => $id], '*', MUST_EXIST);
if (!array_key_exists($id, static::$instances) && !PHPUNIT_TEST) {
static::$instances[$id] = $DB->get_record(tables::INSTANCE_TABLE, ['id' => $id], '*', MUST_EXIST);
}
return static::$instances[$id];
}
Expand All @@ -127,14 +113,19 @@ public static function is_ready($verbalfeedbackid) {
*/
public static function get_instance_by_itemid($itemid) {
global $DB;
return $DB->get_record_sql("SELECT *
FROM {" . tables::INSTANCE_TABLE .
"} WHERE id = (SELECT instanceid
FROM {" . tables::INSTANCE_CATEGORY_TABLE .
"} WHERE id = (SELECT categoryid
FROM {" . tables::INSTANCE_CRITERION_TABLE .
"} WHERE id = ?))",
[$itemid], IGNORE_MISSING);
[$vftable, $cattable, $crittable] = [tables::INSTANCE_TABLE, tables::INSTANCE_CATEGORY_TABLE, tables::INSTANCE_CRITERION_TABLE];
$id = (int)$itemid;
static $instances = [];
if (isset($instances[$id])) {
return $instances[$id];
}
$instances[$id] = $DB->get_record_sql("
SELECT * FROM {{$vftable} vf
INNER JOIN {{$cattable}} cat ON cat.instanceid = vf.id
INNER JOIN {{$crittable}} crit ON crit.categoryid = cat.id
WHERE crit.id = ?",
[$id], IGNORE_MISSING);
return $instances[$id];
}

/**
Expand All @@ -146,12 +137,18 @@ public static function get_instance_by_itemid($itemid) {
*/
public static function get_instance_by_categoryid($categoryid) {
global $DB;
return $DB->get_record_sql("SELECT *
FROM {" . tables::INSTANCE_TABLE .
"} WHERE id = (SELECT instanceid
FROM {" . tables::INSTANCE_CATEGORY_TABLE .
"} WHERE id = ?)",
[$categoryid], IGNORE_MISSING);
[$vftable, $cattable] = [tables::INSTANCE_TABLE, tables::INSTANCE_CATEGORY_TABLE];
$id = (int)$categoryid;
static $instances = [];
if (isset($instances[$id])) {
return $instances[$id];
}
$instances[$id] = $DB->get_record_sql("
SELECT * FROM {{$vftable}} vf
INNER JOIN {{$cattable}} cat ON cat.instanceid = vf.id
WHERE cat.id = ?",
[$id], IGNORE_MISSING);
return $instances[$id];
}

/**
Expand Down Expand Up @@ -237,7 +234,7 @@ public static function generate_verbalfeedback_feedback_states($verbalfeedbackid
global $DB;
$submissionrepo = new submission_repository();

$verbalfeedback = $DB->get_record(self::DB_INSTANCE, ['id' => $verbalfeedbackid], '*', MUST_EXIST);
$verbalfeedback = self::get_instance($verbalfeedbackid);
$wheres = [
'u.id NOT IN (
SELECT fs.touserid
Expand Down Expand Up @@ -309,42 +306,29 @@ function($user) {
*/
public static function get_scales() {

$s0 = new stdClass();
$s0->scale = null;
$s0->scalelabel = get_string('notapplicableabbr', 'verbalfeedback');
$s0->description = get_string('scalenotapplicable', 'mod_verbalfeedback');

$s1 = new stdClass();
$s1->scale = 0;
$s1->scalelabel = '0';
$s1->description = get_string('scalestronglydisagree', 'mod_verbalfeedback');

$s2 = new stdClass();
$s2->scale = 1;
$s2->scalelabel = '1';
$s2->description = get_string('scaledisagree', 'mod_verbalfeedback');

$s3 = new stdClass();
$s3->scale = 2;
$s3->scalelabel = '2';
$s3->description = get_string('scalesomewhatdisagree', 'mod_verbalfeedback');

$s4 = new stdClass();
$s4->scale = 3;
$s4->scalelabel = '3';
$s4->description = get_string('scalesomewhatagree', 'mod_verbalfeedback');

$s5 = new stdClass();
$s5->scale = 4;
$s5->scalelabel = '4';
$s5->description = get_string('scaleagree', 'mod_verbalfeedback');

$s6 = new stdClass();
$s6->scale = 5;
$s6->scalelabel = '5';
$s6->description = get_string('scalestronglyagree', 'mod_verbalfeedback');

return [$s1, $s2, $s3, $s4, $s5, $s6, $s0];
$wordscale = [
'scalestronglydisagree',
'scaledisagree',
'scalesomewhatdisagree',
'scalesomewhatagree',
'scaleagree',
'scalestronglyagree',
];
$scales = [];
for ($i = 0; $i <= 5; $i++) {
$scales[] = (object)[
'scale' => $i,
'scalelabel' => '' . $i,
'description' => get_string($wordscale[$i], 'mod_verbalfeedback'),
];
}
$scales[] = (object)[
'scale' => null,
'scalelabel' => get_string('notapplicableabbr', 'verbalfeedback'),
'description' => get_string('scalenotapplicable', 'mod_verbalfeedback'),
];

return $scales;
}

/**
Expand Down Expand Up @@ -463,7 +447,7 @@ public static function count_users_awaiting_feedback($verbalfeedbackid, $user) {

// Check first if the user can write feedback to other participants.
if (user_utils::can_respond($verbalfeedback, $user) === true) {
if (!$DB->record_exists(self::DB_SUBMISSION, ['instance' => $verbalfeedback->id, 'fromuser' => $user])) {
if (!$DB->record_exists(tables::SUBMISSION_TABLE, ['instance' => $verbalfeedback->id, 'fromuser' => $user])) {
// Generate submission records if there are no submission records yet.
self::generate_verbalfeedback_feedback_states($verbalfeedback->id, $user, $verbalfeedback->with_self_review);
}
Expand All @@ -473,7 +457,7 @@ public static function count_users_awaiting_feedback($verbalfeedbackid, $user) {
$select = "instance = :verbalfeedback AND fromuser = :fromuser AND status $insql";
$params['verbalfeedback'] = $verbalfeedbackid;
$params['fromuser'] = $user;
return $DB->count_records_select(self::DB_SUBMISSION, $select, $params);
return $DB->count_records_select(tables::SUBMISSION_TABLE, $select, $params);
}

return 0;
Expand Down
5 changes: 3 additions & 2 deletions classes/repository/instance_repository.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
defined('MOODLE_INTERNAL') || die();

use Exception;
use mod_verbalfeedback\api;
use mod_verbalfeedback\model\instance;
use mod_verbalfeedback\model\instance_category;
use mod_verbalfeedback\model\instance_status;
Expand All @@ -46,7 +47,7 @@ class instance_repository {

/**
* Gets the instance with the given id
* @param int $id The language id.
* @param int $id The verbal feedback id.
* @return instance|null The language.
*/
public static function get_by_id(int $id): instance {
Expand All @@ -58,7 +59,7 @@ public static function get_by_id(int $id): instance {
return $byid[$id];
}

$dboinstance = $DB->get_record(tables::INSTANCE_TABLE, ["id" => $id]);
$dboinstance = api::get_instance($id);
$instance = db_instance::to_instance($dboinstance);

$dbocategories = $DB->get_records(tables::INSTANCE_CATEGORY_TABLE, ["instanceid" => $id]);
Expand Down
2 changes: 1 addition & 1 deletion classes/repository/submission_repository.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public function save(submission $submission) {
*/
public function delete_by_id(int $id): bool {
global $DB;
return $DB->delete_records('verbalfeedback_language', ['id' => $id]);
return $DB->delete_records(tables::LANGUAGE_TABLE, ['id' => $id]);
}

/**
Expand Down
4 changes: 4 additions & 0 deletions classes/repository/tables.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ class tables {
*
*/
const TEMPLATE_TABLE = 'verbalfeedback_template';
/**
* Template category table name.
*/
const TEMPLATE_CATEGORY_TABLE = 'verbalfeedback_t_category';
/**
*
*/
Expand Down
28 changes: 14 additions & 14 deletions classes/repository/template_category_repository.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public function get_all(): array {
}

$results = [];
$rs = $DB->get_recordset("verbalfeedback_t_category");
$rs = $DB->get_recordset(tables::TEMPLATE_CATEGORY_TABLE);
$templatecategories = [];
foreach ($rs as $dbocategory) {
$templatecategory = db_template_category::to_template_category($dbocategory);
Expand Down Expand Up @@ -97,10 +97,10 @@ public function save(template_category $templatecategory) {
$transaction = $DB->start_delegated_transaction();
$dbocategory = db_template_category::from_template_category($templatecategory);
if ($templatecategory->get_id() === null || $templatecategory->get_id() == 0) {
$id = $DB->insert_record('verbalfeedback_t_category', $dbocategory);
$id = $DB->insert_record(tables::TEMPLATE_CATEGORY_TABLE, $dbocategory);
$templatecategory->set_id($id);
} else {
$DB->update_record('verbalfeedback_t_category', $dbocategory);
$DB->update_record(tables::TEMPLATE_CATEGORY_TABLE, $dbocategory);
}

// Insert or update category headers.
Expand All @@ -109,18 +109,18 @@ public function save(template_category $templatecategory) {
localized_string_type::TEMPLATE_CATEGORY_HEADER, $templatecategory->get_id());

if ($header->get_id() == 0) {
$DB->insert_record('verbalfeedback_local_string', $dboheader);
$DB->insert_record(tables::LOCALIZED_STRING_TABLE, $dboheader);
} else {
$DB->update_record('verbalfeedback_local_string', $dboheader);
$DB->update_record(tables::LOCALIZED_STRING_TABLE, $dboheader);
}
}

// Update linked criteria.
$DB->delete_records('verbalfeedback_t_param_crit', ['categoryid' => $templatecategory->get_id()]);
$DB->delete_records(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE, ['categoryid' => $templatecategory->get_id()]);
foreach ($templatecategory->get_template_criteria() as $criterion) {
$dboparamcriterion = db_parametrized_criterion::from_parametrized_criterion($criterion,
$templatecategory->get_id());
$DB->insert_record('verbalfeedback_t_param_crit', $dboparamcriterion);
$DB->insert_record(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE, $dboparamcriterion);
}

$transaction->allow_commit();
Expand All @@ -140,10 +140,10 @@ public function delete_by_id(int $id): bool {
global $DB;
try {
$transaction = $DB->start_delegated_transaction();
$DB->delete_records('verbalfeedback_local_string', ['foreignkey' => $id,
$DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $id,
'type' => localized_string_type::TEMPLATE_CATEGORY_HEADER, ]);
$DB->delete_records('verbalfeedback_t_param_crit', ['categoryid' => $id]);
$DB->delete_records('verbalfeedback_t_category', ['id' => $id]);
$DB->delete_records(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE, ['categoryid' => $id]);
$DB->delete_records(tables::TEMPLATE_CATEGORY_TABLE, ['id' => $id]);

$transaction->allow_commit();
} catch (Exception $e) {
Expand All @@ -162,7 +162,7 @@ public function delete_by_id(int $id): bool {
private function get_headers($foreignkey): array {
global $DB;

$dboheaders = $DB->get_records('verbalfeedback_local_string',
$dboheaders = $DB->get_records(tables::LOCALIZED_STRING_TABLE,
['foreignkey' => $foreignkey, 'type' => localized_string_type::TEMPLATE_CATEGORY_HEADER]);
$headers = [];
foreach ($dboheaders as $dboheader) {
Expand All @@ -182,7 +182,7 @@ private function get_headers_by_category_ids(): array {
global $DB;

$headers = [];
$rs = $DB->get_recordset('verbalfeedback_local_string', ['type' => localized_string_type::TEMPLATE_CATEGORY_HEADER]);
$rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['type' => localized_string_type::TEMPLATE_CATEGORY_HEADER]);
foreach ($rs as $row) {
if (!\array_key_exists($row->foreignkey, $headers)) {
$headers[$row->foreignkey] = [];
Expand All @@ -203,7 +203,7 @@ private function get_headers_by_category_ids(): array {
private function get_parametrized_criteria($categoryid): array {
global $DB;

$dboparametrizedcriteria = $DB->get_records('verbalfeedback_t_param_crit', ['categoryid' => $categoryid]);
$dboparametrizedcriteria = $DB->get_records(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE, ['categoryid' => $categoryid]);
$parametrizedcriteria = [];
foreach ($dboparametrizedcriteria as $dboparametrizedcriterion) {
$parametrizedcriteria[] = db_parametrized_criterion::to_parametrized_criterion($dboparametrizedcriterion);
Expand All @@ -220,7 +220,7 @@ private function get_parameterized_criteria_by_categoryid(): array {
global $DB;

$critbycatid = [];
$rs = $DB->get_recordset('verbalfeedback_t_param_crit');
$rs = $DB->get_recordset(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE);
foreach ($rs as $row) {
if (!\array_key_exists($row->categoryid, $critbycatid)) {
$critbycatid[$row->categoryid] = [];
Expand Down

0 comments on commit 4dec38c

Please sign in to comment.