How to get ideas for a gem, when you don't know what to make

code
ruby
resque-scheduler-web

5 May 2015

Ruby is currently (and will likely remain) my favourite ever coding language, so I really want to get to know every part of it as well as I can. This has given me an itch to learn how to create and publish gems for quite a while, so recently I decided to give it a try. I've gained a lot from the enormous array of community-written gems that people have made available, so becoming part of the community and contributing something back felt like a worthy thing to aspire to.

However, when I sat down to get started I ran into a problem before I could even begin: what do I make?

At the time, I didn't have a project that seemed like a good candidate for being an independent gem, and especially not one that other people might want to use. So what to do if you want to get something out there and you're stuck for inspiration? Well, here's the options as I see them:

  1. Scan over all your recent work, whether it's for major projects, or just messing about, and look for duplication. Anything that's occurring more than once is a candidate for becoming a gem. This could be something really simple. For example, a bunch of scopes and mixed-in methods that you use in more than one active record model. You could add them to any active record class by calling a class method that your gem can add to ActiveRecord::Base e.g. the way the searchkick gem does, allowing you to do this:

    # In the gem
ActiveRecord::Base.send(:extend, Searchkick::Model) if defined?(ActiveRecord)

     # In your code
 class Product < ActiveRecord::Base
   searchkick
 end

    You may feel you'd be better served by making a module to mix in within your app to start with. However, gems don't necessarily need to be public - you can install them from a local folder on your filesystem, or from a gihub repo (public or private).

     gem 'my_gem', github: 'myaccount/my_gem'
 gem 'my_other_gem', path: '/Users/me/code/my_other_gem'

    Not a bad way to practice before doing something bigger.

  2. Imagine a cool extra feature that any gem you're using could have, then make a gem that monkey-patches the new feature into existence. This is the approach that Resque Scheduler takes. The main Resque class ends up with a load of new stuff mixed in from a module in the gem:

    # Within the gem
Resque.extend Resque::Scheduler::Extension

    # In your code
Resque.enqueue_at(1.day.from_now, SendReminderEmails)

  3. Look at the gems you are currently using. Do any of them have a plugin structure where third party gems can add functionality? Resque Web, for example, is currently structured like this, so extending the interface with a new tab page is easily done by packing up a small Rails Engine into a new gem. If you find a gem like this, then there may be an easy-ish suggestion or two in the issue tracker. Find the gem on Rubygems, click on the issues link (which will probably go to the issues page for the gem's Github repo), and see what's there. Alternatively, ask the maintainers directly.

This third approach worked for me, when I realised that Resque Web had recently been extracted into a separate gem ready for Resque 2.x (rather than being bundled with Resque 1.x), and moved from a Sinatra app to being a Rails engine. This included a new way to define plugins cleanly via Rails Engines that could be mounted inside the Resque Web Engine itself. Perfect! I noticed that various other Resque-related gems which provide extensions to Resque Web had not been updated yet, so their old Sinatra code would need to be ported over to a Rails engine in order to be usable with the new gem. Given that one of these was resque scheduler, which I wanted to use in a project, this seemed like an ideal bit of open source work to have a go at.

Anyway, there is now a nice shiny new gem called resque-scheduler-web, which works very well, and which seems to be popular. I'm very proud of it, although it's taken a lot more work than I thought!

Things I learned building the gem:

Sometimes extracting part of an existing gem into a new gem can be a good thing.

The strange thing is that I didn't initially think of making this as a good project to make into a gem. I needed the functionality, so I just made a pull request for the main Resque Scheduler gem with the code and tests refactored as a Rails engine. This made sense at the time, as I was working on code which already contained the functionality for the resque web extension (as well as the code for the actual scheduler), so why not just update it in place? However, as one of the Resque Scheduler maintainers pointed out, it seems to work better as a separate gem. Firstly, this is because it is intrinsically doing a different job, so it's better if the two gems can vary independently. And secondly, because the old Sinatra interface is still the default for Resque 1.x, so we should not yet replace the Sinatra code in the current Resque Scheduler gem, which is still the recommended interface.

Gem maintainers can provide valuable insights you may not otherwise think of.

Had I not had a discussion in a pull request thread on Github about it, I may not have realised that making a new gem was a better idea, and more convenient for people. Good thing I did! Also, gem maintainers in my experience seem pretty friendly, and appreciate it if you reach out and offer to help. If you are not sure of whether or how to improve an existing gem by making a plugin of some sort, reach out and ask the maintainers about it. They may well point you in a better direction than you would have come up with yourself, making your life easier and your gem more popular.

Test coverage is everything. Aim for 100%, and take test refactoring into account.

Releasing a gem means that people are inevitably going to end up using it in their projects, so not having solid test coverage puts the success of their work at risk. Trouble is, whilst moving the code and tests over to the new gem and porting them to Rspec, it became clear that the coverage was far from perfect. Some entire pages had nothing but an integration test asserting that the response was a 200 status code! I couldn't leave things like that, so I decided to write a full set of integration tests covering all of the existing functionality. Unfortunately, this made the whole project take quite a lot longer than I had envisaged, so make sure you take this into account when starting out.

Conclusion

Overall, making my first gem was a great experience, but could certainly have been an easier ride.

0 Comments
Mountain Road logo

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