Core renderer public function outputting simple array - any risks?

Core renderer public function outputting simple array - any risks?

by Andrew C -
Number of replies: 4

Hi Everyone,

I've written a function that produces an array to solve my problem from here: https://moodle.org/mod/forum/discuss.php?d=356256

I have an array similar to:

Array
1 image1.jpg
2 image2.jpg
3 image3.jpg
4 image4.jpg


in my core_renderer as a public function

and call this array in my layout files via:

$patharray = $OUTPUT->get_imagepaths();

Is passing an array generated from a function via public method a bad idea? if so, why?


I intend to then construct the path using the filename from the array in the layout page.


Thanks,

Andrew

Average of ratings: -
In reply to Andrew C

Re: Core renderer public function outputting simple array - any risks?

by Gareth J Barnard -
Picture of Core developers Picture of Particularly helpful Moodlers Picture of Plugin developers

Depends if you can refactor the code to generate the output from the data contained in the array within the core_renderer, then that method would be public but the method get_imagepaths() could be protected otherwise no choice but public.  All depends on your OO design and encapsulation strategy.

Average of ratings: -
In reply to Gareth J Barnard

Re: Core renderer public function outputting simple array - any risks?

by Andrew C -

Thanks Gareth. Really interesting.

The $output part of the function contains the results of a $DB->get_records_sql call from a very basic SELECT query from a single custom table I've created. E.g.

$results = $DB->get_records_sql($sql);

$output = $results;

return $output;

Do you know if it's possible to refactor the $DB->get_records_sql call (or any other data)? I should add that I'm not using any other globals in the function other than db and all full paths will be constructed away from the filename held in the array.

Andrew

Average of ratings: -
In reply to Andrew C

Re: Core renderer public function outputting simple array - any risks?

by Gareth J Barnard -
Picture of Core developers Picture of Particularly helpful Moodlers Picture of Plugin developers

Hi Andrew,

No I don't but I always look to see if the data is already in a core object / variable / getter method as its likely to be cached rather than another database call.  Can you get the information you need from the existing tables or is it bespoke to your theme?  And see if you can use 'get_records' with an array of field criteria's instead of pure SQL.

G

Average of ratings: -
In reply to Andrew C

Re: Core renderer public function outputting simple array - any risks?

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

When implementing new functionality, renderers should only contain code to output HTML based on the arguments that werre passed in. Don't do anything in a renderer method that you would not do in a template.

When you are overriding an existing renderer, in order to change some existing bit of Moodle output. Well, then you have less control. The renderer base class controls which arguments are passed into the method. If you need more data, then you have to break the rules and get it.

In an ideal world, you put the logic to get that data into some other object or fuction, so all the renderer has to do is call it.

If what you are working on is a quick and dirty solution to somthing, then just put the $DB->get_records call into the renderer method. Sometimes life is too short to do anything else.



Average of ratings: -