How to refactor Moodle code so that it's easy to understand and maintain using the composed method pattern

moodle
code
refactoring
patterns

15 April 2015

Having used Moodle for many years, I've found that the code can sometimes be a bit unwieldy in places and can be rather confusing to read (including my own!). This isn't a lot of fun. Thankfully, there are many techniques for refactoring code in order to make it more readable and maintainable. This post will outline the composed method pattern, which is one of my favourites, and show you how you can use it. We'll be taking part of a bloated class that's doing too much and refactoring it into an easy to understand class of its own.

The Composed Method Pattern

This was first described by Kent Beck, and is a great way to get code to be a lot more readable and maintainable for very little extra effort. It's a recognised best practice for coders and very common in the Ruby world, but doesn't seem to get used much within Moodle :)

It's a simple idea: you follow three basic principles to make large methods easy to understand and work with, by breaking them down into smaller ones:

  1. Methods should be composed of calls to other methods, each of which does only one thing This is the same as the single responsibility principle that you should be applying to classes. In terms of methods, this means you will often have methods that are only one line long, and ideally should always be 5 lines or fewer.
  2. Methods should deal with only one level of abstraction Often, we read code and see a loop or an if/else block and have to stop and work out exactly what the inner bit is doing. This slows us down and can take up a lot of space, so this principle says that we always replace the contents of these blocks with a method call. Even if the method is only one line.
  3. Methods should be small. Often, only one line long, and ideally never more than 5. If you follow the first two principles, this will happen naturally. You'll end up with more methods than usual, but each one is instantly understandable and easy to reuse.

A corollary to these principles is that method names should be clear and descriptive. This allows us to know exactly what is going on and is important if you have lots of small methods with very specific single purposes. Short vague names create the potential for confusion. When done correctly, this also means that you no longer need to add comments to your code as it's always perfectly clear from the method name what is happening. Often this will mean using method names that are longer and more verbose than you may be used to, but the advantage is that your code reads a lot like plain English, is faster to understand for yourself and others, and the descriptions never get out of sync with what the code is doing (as comments often tend to do).

An example

To illustrate how this can help complex code to get clearer, here's one function in the assign class in the assignment module locallib.php that doesn't use it:

/**
 * Save assignment submission for the current user.
 *
 * @param  stdClass $data
 * @param  array $notices Any error messages that should be shown
 *                        to the user.
 * @return bool
 */
public function save_submission(stdClass $data, & $notices) {
    global $CFG, $USER;

    require_capability('mod/assign:submit', $this->context);
    $instance = $this->get_instance();

    if ($instance->teamsubmission) {
        $submission = $this->get_group_submission($USER->id, 0, true);
    } else {
        $submission = $this->get_user_submission($USER->id, true);
    }
    if ($instance->submissiondrafts) {
        $submission->status = ASSIGN_SUBMISSION_STATUS_DRAFT;
    } else {
        $submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED;
    }

    $flags = $this->get_user_flags($USER->id, false);

    // Get the flags to check if it is locked.
    if ($flags && $flags->locked) {
        print_error('submissionslocked', 'assign');
        return true;
    }

    $pluginerror = false;
    foreach ($this->submissionplugins as $plugin) {
        if ($plugin->is_enabled() && $plugin->is_visible()) {
            if (!$plugin->save($submission, $data)) {
                $notices[] = $plugin->get_error();
                $pluginerror = true;
            }
        }
    }
    $allempty = $this->submission_empty($submission);
    if ($pluginerror || $allempty) {
        if ($allempty) {
            $notices[] = get_string('submissionempty', 'mod_assign');
        }
        return false;
    }

    $this->update_submission($submission, $USER->id, true, $instance->teamsubmission);

    // Logging.
    if (isset($data->submissionstatement)) {
        $logmessage = get_string('submissionstatementacceptedlog',
                                 'mod_assign',
                                 fullname($USER));
        $this->add_to_log('submission statement accepted', $logmessage);
        $addtolog = $this->add_to_log('submission statement accepted', $logmessage, '', true);
        $params = array(
            'context' => $this->context,
            'objectid' => $submission->id
        );
        $event = \mod_assign\event\statement_accepted::create($params);
        $event->set_legacy_logdata($addtolog);
        $event->trigger();
    }
    $addtolog = $this->add_to_log('submit', $this->format_submission_for_log($submission), '', true);
    $params = array(
        'context' => $this->context,
        'objectid' => $submission->id
    );
    $event = \mod_assign\event\submission_updated::create($params);
    $event->set_legacy_logdata($addtolog);
    $event->trigger();

    $complete = COMPLETION_INCOMPLETE;
    if ($submission->status == ASSIGN_SUBMISSION_STATUS_SUBMITTED) {
        $complete = COMPLETION_COMPLETE;
    }
    $completion = new completion_info($this->get_course());
    if ($completion->is_enabled($this->get_course_module()) && $instance->completionsubmit) {
        $completion->update_state($this->get_course_module(), $complete, $USER->id);
    }

    if (!$instance->submissiondrafts) {
        $this->notify_student_submission_receipt($submission);
        $this->notify_graders($submission);
        // Trigger assessable_submitted event on submission.
        $params = array(
            'context' => context_module::instance($this->get_course_module()->id),
            'objectid' => $submission->id,
            'other' => array(
                'submission_editable' => true
            )
        );
        $event = \mod_assign\event\assessable_submitted::create($params);
        $event->trigger();
    }
    return true;
}

Why is this code bad? Here are a few reasons:

  • The function is too big. There are many responsibilities in here, so each action being taken cannot be reused elsewhere.
  • This code is hard to unit test. The different pathways through it, and the dependencies on the other parts of the containing class mean that our tests will need complicated setup phases to make sure all the branches get exercised. Not good!
  • It's confusing. Good code should read like plain English, and this really doesn't. Working out the steps it's taking is going to require a fair bit of reading and thinking, which slows down maintenance and refactoring work.
  • It's hard to refactor. The code is littered with temporary variables, so we are going to find that moving stuff around into new methods and classes we will no longer have access to these variables. We may end up passing loads of variables in and out of functions and creating confusion, or maybe we will get discouraged by the complexity and just won't bother.
  • It's not fun. Coding can be a very relaxing experience where we are absorbed in what we are doing. A state of mind that's been termed 'Flow'. Complicated, messy code which takes a lot of effort to read makes this harder and breaks the pleasant sense of absorption.

So... Let's fix it!

Step 1: Make a new class

Looking at the context makes it clear that this method doesn't seem to belong in the assign class. This function starts on line 5391 of /mod/assign/locallib.php, with nothing else in the file apart from the assign class up until that point and quite a lot more of it yet to come. Clearly this class is doing too much and this function really looks like it should be a class of its own with the responsibility of saving a submission. This will make it far easier to work with. Encapsulating a job to be done in a class which does it for you is a pattern known as a service object. This allows you to keep other classes like assign from becoming a dumping ground for anything that seems vaguely related to it (an anti-pattern called the God object). It also leads to us having many small classes rather than one or two big ones, which makes it far easier to find the code you are looking for, and isolates the code that performs each discrete function so bugs don't affect other areas of our code when they happen.

I've put the method into its own class, passed in the original assign object to the constructor, and changed the old calls to $this->method() to $this->assign->method(). Some of these methods are private, and would need some refactoring work, but we'll ignore that for this exercise as we're just focusing on how the code is structured when refactor this one function.

class submission_saver {

    /**
     * @var \assign
     */
    protected $assign;

    /**
     * @param \assign $assign
     */
    public function __construct($assign) {
        $this->asign = $assign;
    }

    /**
     * @return \assign
     */
    protected function assign() {
        return $this->assign;
    }

    /**
     * Save assignment submission for the current user.
     *
     * @param stdClass $data
     * @param array $notices Any error messages that should be shown
     *                        to the user.
     * @return bool
     */
    public function save_submission(stdClass $data, & $notices) {
        global $CFG, $USER;

        require_capability('mod/assign:submit', $this->context);
        $instance = $this->assign()->get_instance();

        // Rest of the original function here...
    }

}

Step 2: Remove temporary variables

This a necessary pre-requisite to any refactoring work, as otherwise the code in the new functions we are going to make cannot get to the data that it needs when we move it around the class. For example, the $submission variable is created at the top of the original function, then used by some code lower down. This means that refactoring the lower-down code into another function requires at the very least passing the submission into that function and then getting it back. This can quickly become convoluted and messy. If we move the bit that creates the $submission value into its own function, and replace all uses of $submission with $this->submission(), then we never have to worry about it as the function call works from anywhere in the class. This is nicely summed up as "make the change easy, then make the easy change".

// Beginning of class...

public function save_submission() {

   // Start of function...

   $this->update_submission($this->submission(), $USER->id, true, $instance->teamsubmission);

   // Rest of function...

}

/**
* @return stdClass
*/
protected function submission() {
    global $USER;
    if ($this->instance()->teamsubmission) {
        $submission = $this->assign()->get_group_submission($USER->id, 0, true);
    } else {
         $submission = $this->assign()->get_user_submission($USER->id, true);
    }
    if ($this->instance()->submissiondrafts) {
         $submission->status = ASSIGN_SUBMISSION_STATUS_DRAFT;
         return $submission;
    } else {
         $submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED;
         return $submission;
    }
}

/**
 * @return stdClass
 * @throws \coding_exception
 */
protected function instance() {
    return $this->assign()->get_instance();
}

// Rest of class...

Note how the instance() function has been given the same treatment, which means that we no longer have to use the $instance variable from the original function, so submission() now needs no parameters.

We're now free to keep refactoring further by taking even smaller parts of the method and making new methods if we need to. Removing temporary variables like this is a very powerful change to make, but often overlooked when classes are too large, as keeping track of the many small methods can become confusing. They also may begin to interfere with each other. For example, instance() is not a very descriptive name, so we should fix this later in order to prevent this sort of confusion. We might also find that different methods need the same variables to be initialised in different ways, so not as easy in a big class.

Step 3: memoisation - always prefer functions over variables

Memoisation on Wikipedia

Memoisation is the practice of creating a function that will return a single stored variable, or initialise it if it's not there. You use it as a way to speed things up if the instantiation process is expensive, and/or if you want to lazy-load a variable and have it persist afterwards to keep its state. Here, we are using the submission() function instead of using $this->submission directly. The advantage is that if anything need to change about how the variable is set up, we can alter the set_submission() function, and if we need to alter where it comes from in the first place, we can change submission(). In both cases, the rest of the code in the class does not need to change, making our refactorings easier.

// Beginning of class...

/**
 * @var stdClass
 */
protected $submission;

/**
 * @return stdClass
 */
protected function submission() {
    if (!$this->submission) {
        $this->set_submission();
    }
    return $this->submission;
}

/**
 * @return stdClass
 */
private function set_submission() {
    global $USER;

    if ($this->instance()->teamsubmission) {
        $submission = $this->assign()->get_group_submission($USER->id, 0, true);
    } else {
        $submission = $this->assign()->get_user_submission($USER->id, true);
    }
    if ($this->instance()->submissiondrafts) {
        $submission->status = ASSIGN_SUBMISSION_STATUS_DRAFT;
        return $submission;
    } else {
        $submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED;
        return $submission;
    }
}

// Rest of class...

Step 4: methods should only do one thing

The set_submission() method is really doing two things: getting the right submission object and setting the status on it. These should be in different functions, so we make the split:

// Beginning of class...

/**
 * @return stdClass
 */
protected function submission() {
    if (!$this->submission) {
        $this->set_submission();
        $this->set_submission_status();
    }
    return $this->submission;
}

/**
 * @return stdClass
 */
private function set_submission() {
    if ($this->instance()->teamsubmission) {
        $submission = $this->assign()->get_group_submission($USER->id, 0, true);
    } else {
        $submission = $this->assign()->get_user_submission($USER->id, true);
    }
}

private function set_submission_status() {
    if ($this->instance()->submissiondrafts) {
        $submission->status = ASSIGN_SUBMISSION_STATUS_DRAFT;
        return $submission;
    } else {
        $submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED;
        return $submission;
    }
}

// Rest of class...

Step 5: Only use one level of abstraction

This means moving anything inside an if block or a loop into its own method. You may well find this leaves you with methods that are just one line. This is good! Look how much easier it is to read what the set_submission() and set_submission_status() methods are doing. There's no way you'll get confused there now!

// Beginning of class...

/**
 * @return stdClass
 */
protected function submission() {
    if (!$this->submission) {
        $this->set_submission();
        $this->set_submission_status();
    }
    return $this->submission;
}

/**
 * @return stdClass
 */
private function set_submission() {
    if ($this->assignment_has_group_submissions_enabled()) {
        $this->submission = $this->get_group_submission();
    } else {
        $this->submission = $this->get_user_submission();
    }
}

private function set_submission_status() {
    if ($this->assignment_settings_allow_draft_submissions()) {
        $this->set_submission_status_to_draft();
    } else {
        $this->set_submission_status_to_submitted();
    }
}

/**
 * @return stdClass
 */
private function get_group_submission() {
    global $USER;
    return $this->assign()->get_group_submission($USER->id, 0, true);
}

/**
 * @return stdClass
 */
private function get_user_submission() {
    global $USER;
    return $this->assign()->get_user_submission($USER->id, true);
}

private function set_submission_status_to_draft() {
    $this->submission()->status = ASSIGN_SUBMISSION_STATUS_DRAFT;
}

private function set_submission_status_to_submitted() {
    $this->submission()->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED;
}

/**
 * @return mixed
 */
private function assignment_settings_allow_draft_submissions() {
    return $this->instance()->submissiondrafts;
}

/**
 * @return mixed
 */
private function assignment_has_group_submissions_enabled() {
    return $this->instance()->teamsubmission;
}

// Rest of class...

This code now has a lot more functions, but each one is instantly understandable because it does only one thing and the name is very expressive. The general shape of the code is recognisably top-heavy - the complex wiring is higher up, and the small 1 or 2 line components are down the bottom.

The end result

Lets see what the whole class looks like after this treatment. In particular, look at save_submission() and see how long it takes you to work out what's going on.

/**
 * Class submission_saver is responsible for saving the newly submitted user data
 * as a new submission in the database and performing the necessary associated tasks.
 *
 * @package mod_assign
 */
class submission_saver {

    /**
     * @var \assign
     */
    protected $assign;

    /**
     * @var stdClass
     */
    protected $submission;

    /**
     * @var bool
     */
    protected $pluginerror = false;

    /**
     * @var array
     */
    protected $plugin_error_notices = array();

    /**
     * @var \completion_info
     */
    protected $completion_info;

    /**
     * @param \assign $assign
     */
    public function __construct($assign) {
        $this->asign = $assign;
    }

    /**
     * Save assignment submission for the current user.
     *
     * @param stdClass $data
     * @param array $notices Any error messages that should be shown
     *                        to the user.
     * @return bool
     */
    public function save_submission(stdClass $data, & $notices) {
        global $USER;

        $this->plugin_error_notices = $notices;

        $this->ensure_the_user_has_permission_to_submit_in_this_context();

        if ($this->submissions_are_locked_for_this_user()) {
            print_error('submissionslocked', 'assign');
            return true;
        }

        $this->save_data_for_each_assignment_plugin($data);
        $this->add_an_error_notice_if_the_submission_was_empty();

        if ($this->errors_occurred_when_saving_plugin_data() || $this->submission_was_empty()) {
            return false;
        }

        $this->update_gradebook_with_new_submission_data();

        if ($this->the_user_just_accepted_the_submission_terms_statement($data)) {
            $this->log_that_the_student_accepted_the_submission_terms();
        }
        $this->log_that_the_student_made_a_submission();

        if ($this->this_assignment_has_activity_completion_enabled() &&
            $this->students_must_submit_to_this_assignment_in_order_to_complete_it()) {

            $this->update_current_completion_status_for_this_submission($USER);
        }

        if (!$this->assignment_settings_allow_draft_submissions()) {
            $this->trigger_events_and_notifications_that_a_submission_has_happened($this->submission());
        }
        return true;
    }

    /**
     * @return \assign
     */
    protected function assign() {
        return $this->assign;
    }

    /**
     * @param $submission
     * @throws \coding_exception
     */
    protected function trigger_events_and_notifications_that_a_submission_has_happened($submission) {
        $this->assign()->notify_student_submission_receipt($submission);
        $this->assign()->notify_graders($submission);
        // Trigger assessable_submitted event on submission.
        $params = array(
            'context' => \context_module::instance($this->assign()->get_course_module()->id),
            'objectid' => $submission->id,
            'other' => array(
                'submission_editable' => true
            )
        );
        $event = \mod_assign\event\assessable_submitted::create($params);
        $event->trigger();
    }

    /**
     * @return stdClass
     */
    protected function submission() {
        if (!$this->submission) {
            $this->set_submission();
            $this->set_submission_status();
        }
        return $this->submission;
    }

    /**
     * @return stdClass
     */
    private function set_submission() {
        if ($this->assignment_has_group_submissions_enabled()) {
            $this->submission = $this->get_group_submission();
        } else {
            $this->submission = $this->get_user_submission();
        }
    }

    private function set_submission_status() {
        if ($this->assignment_settings_allow_draft_submissions()) {
            $this->set_submission_status_to_draft();
        } else {
            $this->set_submission_status_to_submitted();
        }
    }

    /**
     * @return stdClass
     */
    private function get_group_submission() {
        global $USER;
        return $this->assign()->get_group_submission($USER->id, 0, true);
    }

    /**
     * @return stdClass
     */
    private function get_user_submission() {
        global $USER;
        return $this->assign()->get_user_submission($USER->id, true);
    }

    private function set_submission_status_to_draft() {
        $this->submission()->status = ASSIGN_SUBMISSION_STATUS_DRAFT;
    }

    private function set_submission_status_to_submitted() {
        $this->submission()->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED;
    }

    /**
     * @return mixed
     */
    private function assignment_settings_allow_draft_submissions() {
        return $this->instance()->submissiondrafts;
    }

    /**
     * @return mixed
     */
    private function assignment_has_group_submissions_enabled() {
        return $this->instance()->teamsubmission;
    }

    /**
     * @return stdClass
     * @throws \coding_exception
     */
    protected function instance() {
        return $this->assign()->get_instance();
    }

    /**
     * @param stdClass $data
     * @param $plugin
     * @return array
     */
    private function save_data_for_plugin(stdClass $data, $plugin) {
        if (!$plugin->is_enabled() || !$plugin->is_visible()) {
            return;
        }
        if (!$plugin->save($this->submission(), $data)) {
            $this->plugin_error_notices[] = $plugin->get_error();
            $this->pluginerror = true;
        }
    }

    /**
     * @return bool
     */
    private function errors_occurred_when_saving_plugin_data() {
        return $this->pluginerror;
    }

    /**
     * @return bool
     */
    private function submission_was_empty() {
        return $this->assign()->submission_empty($this->submission());
    }

    /**
     * @param stdClass $data
     */
    private function save_data_for_each_assignment_plugin(stdClass $data) {
        foreach ($this->assign()->submissionplugins as $plugin) {
            $this->save_data_for_plugin($data, $plugin);
        }
    }

    private function add_an_error_notice_if_the_submission_was_empty() {
        if ($this->submission_was_empty()) {
            $this->plugin_error_notices[] = get_string('submissionempty', 'mod_assign');
        }
    }

    /**
     * @return stdClass
     */
    private function user_flags() {
        global $USER;
        return $this->assign()->get_user_flags($USER->id, false);
    }

    /**
     * @return mixed
     */
    private function submissions_are_locked_for_this_user() {
        return $this->user_flags() && $this->user_flags()->locked;
    }

    private function ensure_the_user_has_permission_to_submit_in_this_context() {
        require_capability('mod/assign:submit', $this->context);
    }

    /**
     */
    private function update_gradebook_with_new_submission_data() {
        global $USER;
        $this->assign()->update_submission($this->submission(), $USER->id, true, $this->instance()->teamsubmission);
    }

    /**
     * @param stdClass $data
     * @return bool
     */
    private function the_user_just_accepted_the_submission_terms_statement(stdClass $data) {
        return isset($data->submissionstatement);
    }

    /**
     * @return array
     * @throws \coding_exception
     */
    private function log_that_the_student_accepted_the_submission_terms() {
        global $USER;
        $logmessage = get_string('submissionstatementacceptedlog',
                                 'mod_assign',
                                 fullname($USER));
        $this->assign()->add_to_log('submission statement accepted', $logmessage);
        $addtolog = $this->assign()->add_to_log('submission statement accepted', $logmessage, '', true);
        $params = array(
            'context' => $this->assign()->context,
            'objectid' => $this->submission()->id
        );
        $event = \mod_assign\event\statement_accepted::create($params);
        $event->set_legacy_logdata($addtolog);
        $event->trigger();
    }

    private function log_that_the_student_made_a_submission() {
        $addtolog = $this->assign()
            ->add_to_log('submit', $this->assign()->format_submission_for_log($this->submission()), '', true);
        $params = array(
            'context' => $this->context,
            'objectid' => $this->submission()->id
        );
        $event = \mod_assign\event\submission_updated::create($params);
        $event->set_legacy_logdata($addtolog);
        $event->trigger();
    }

    /**
     * @return bool
     */
    private function submission_status_is_final() {
        return $this->submission()->status == ASSIGN_SUBMISSION_STATUS_SUBMITTED;
    }

    /**
     * @return mixed
     */
    private function course() {
        return $this->assign()->get_course();
    }

    /**
     * @return int
     */
    private function activity_completion_status_for_this_submission() {
        $complete = COMPLETION_INCOMPLETE;
        if ($this->submission_status_is_final()) {
            $complete = COMPLETION_COMPLETE;
        }
        return $complete;
    }

    /**
     * @return \completion_info
     */
    private function completion_info() {
        if (!$this->completion_info) {
            $this->completion_info = new \completion_info($this->course());
        }
        return $this->completion_info;
    }

    /**
     * @return mixed
     */
    private function this_assignment_has_activity_completion_enabled() {
        return $this->completion_info()->is_enabled($this->assign()->get_course_module());
    }

    /**
     * @return mixed
     */
    private function students_must_submit_to_this_assignment_in_order_to_complete_it() {
        return $this->instance()->completionsubmit;
    }

    /**
     */
    private function update_current_completion_status_for_this_submission() {
        global $USER;
        $status = $this->activity_completion_status_for_this_submission();
        $this->completion_info()->update_state($this->assign()->get_course_module(), $status, $USER->id);
    }
}

Much better!

The class now does only one thing, with just one public method to call. That method is easy to understand because it calls lots of smaller methods which are all at one level of abstraction and each do only one thing.

We can get what this class does very easily by looking at the few bigger methods at the top, then if we need to refactor anything, we can easily re-compose or alter the smaller methods with a minimum of fuss.

All this makes the code far easier to understand next time we look at it and isolates it from the rest of the system, so changes will not cause bugs elsewhere. It also makes it far easier to unit test, ensuring we catch bugs before they happen.

If only all Moodle code was like this!

0 Comments
Mountain Road logo
1 2