praxisdigital / mod_diplomasafe

Plugin made for interaction with Diplomasafe.com
Other
0 stars 0 forks source link

wrapping global $DB in private $db var not following normal guidelines. #9

Open danmarsden opened 2 years ago

danmarsden commented 2 years ago

There is very little reason to abstract the global $DB var into a private $this->db style var.

it's much cleaner (and following moodle guidelines) to just do: global $DB;

and then access it directly.

I'm also not a fan of having table names in constants either - better to just do:

$DB->insert_record('diplomasafe_languages', object);

than

$this->db->insert_record(self::TABLE, (object)

That follows our conventions a lot better.

I haven't spotted a reason to block approval based on this issue yet though.

danmarsden commented 2 years ago

here's another good example of a pretty useless wrapper you have created on a core function: https://github.com/praxisdigital/mod_diplomasafe/blob/MOODLE_311_STABLE/classes/templates/repository.php#L149 you then create a sql call, but really all you should be doing right there is:

 $record = $DB->get_record("diplomasafe_templates", ['idnumber' => $template_idnumber]);

And... if you want an exception when no record exists like your validate_record() thing you just do:

 $record = $DB->get_record("diplomasafe_templates", ['idnumber' => $template_idnumber], '*', MUST_EXIST);