Refactoring an Overgrown Notifications Class in Django

Gabriela Voicu — Wed, 24 Jan 2018 16:04:02 +0000

After working with the same code base for a while, there comes a time when a developer starts noticing ways that certain parts of it can be improved, both in terms of performance and in terms of code quality. Code also frequently evolves over time to keep up with changing requirements. Oftentimes, these changes can be greatly simplified. In the case of the notifications class in NextTier’s Django API, both were true.

The API handles the interactions of students with their college application tasks and their counselors. Students can find colleges and scholarships to apply to, take tests to help guide them towards a career and keep track of their college application process through tasks specifically tailored for each college. Counselors can follow up with a student’s progress directly through messages and verifying their dashboard, and indirectly through a system of notifications.

The notification sending class has gone through many changes over the past two years, needing to do more and more to keep up with evolving requirements. As we now have a much better idea of what we need in a notifications system, and are starting to feel performance slowdown, it was time to do a ruthless analysis of the notification sending class, SendNotificationMixin.

The Initial State

This is what the notification sending class looked like before the refactor:

The issues I identified and wanted to address with the refactor:

Arguments

send_notification takes in nine arguments, without counting the extra keyword arguments that can be passed in ( **kwargs); this urgently needs to be improved as debugging this method is extremely hard. Trying to isolate which particular argument passed in causes an error or creates a specific piece of text in an email has proven extremely time consuming in the past.

Logs

The method has logs that are very difficult to read and many unnecessary events that add noise to the logs.

Complexity

The send_notificationmethod code tries to do too many things, which makes it extremely convoluted.

The method is essentially in charge of creating three types of notifications:

Notification objects in the database (that serve for the web app center notifications)
Mobile push notifications
Emails

While most events that trigger notifications in the app require all three types of notifications, an increasing number do not, and new requirements for mobile push notifications will mean removing even more push notifications. Based on this I’m planning to split this method into three separate methods that will each handle the creation of one single type of notification, and be called accordingly by methods generated by each event. A method called by an event will be responsible for knowing which types of notifications it needs to create.

Inheritance

view classes inherit from SendNotificationMixin. View classes are not notification classes, so inheritance does not make sense in this scenario. This will be replaced with composition. Though rare, there were a couple of times when inheritance has caused method overwrites.

Arguments (again)

send_notification needs to be called with a very particular set of arguments. Their creation takes up many lines of code inside the view class methods. These add noise and clutter to the views, and will be replaced with a call to a method from SendNotificationMixin that will know how to send the proper notifications for the event created in that view.

Performance

Performance is a big issue for events that require many notifications to be sent, for example if a counselor creates a new task and assigns it to 100 students. From my testing, the problem is caused both by notification sending code and by the actual calls made to outside services. The solution will be two pronged: for the notification sending code, the refactor will also focus on reducing the time complexity of the code; for the calls to the email and push notification sending services, they will be moved to Django’s Channels, an asynchronous task processor.

An Excessively-large Object

An entire Django Request object is passed in from the view methods all the way down to the email sending method to only use some of its properties in some of the methods. Outside of it being an unnecessary to send the entire object for only some properties, this also causes an issue for events that trigger notifications outside of API calls, where there is no HTTP request to pass along.

Inheritance (again)

The class inherits from SendEmailMixin. While an argument could potentially be made about whether or not a notification sending class is a type of email sending class (and therefore inheritance would be an appropriate solution), SendNotificationMixin does not make use of any of the benefits inheritance brings. As long as the only method the notifications class calls from SendEmailMixin is send_email, composition makes more sense. This will also help cleanup more view classes that inherit from the email sending class.

Standardization (or lack thereof)

Initially the app was created by a consultancy, with multiple people working on it which resulted in there being a few different ways in which methods are called and data structures are created. This creates a lot of unnecessary complexity and makes debugging and writing new code more complicated than it needs to be.

Phase 1

In this first part of the refactor, I moved all the notification sending code out of the views, to make them smaller and easier to read. A typical notification sending view looked like this before the refactor:

This POST handles the creation of a completed achievement, sending notifications for that achievement and creating feed events. Lines 30 to 54 handle the sending of the notifications and none of that code needs to be at the top level, visible inside the view method. It will be replaced by a call to the send_completed_phase_achievement_notifications method that lives on the SendNotificationMixin class. Now the post method is a whole lot easier to read:

(There are obviously other things that could be improved in post, but they are outside the scope of the notifications refactor so they will need to be addressed another time.)

I repeated this step for all of the other view classes, and created 22 new methods on SendNotificationMixin that handle the creation of notifications based on specific events.

Phase 2

In this part, we get to remove the HTTP request object that gets passed to send_notification and send_email. Having to pass this object to methods that send notifications makes it really hard to send those notifications for events that happen outside of an HTTP request like a phase migration script, a file upload account creation, or other events. Removing request will also help simplify the methods sending notifications, ensuring they take fewer arguments.

Request is used by send_email to generate a link for a user to click and log in. The object is passed through four or five separate methods to get here:

We are now able to replace all instances in the code that look like this:

context = {
            'invitee': invitee,
            'link': the_right_host(request)
}

With a new environment variable that will hold the different values of the host for the different environments:

context = {
            'invitee': invitee,
            'link': settings.HOST_URL
}

With this change, send_email no longer needs request passed in, which means send_notification also no longer needs request passed in. This allows us to get rid of that object everywhere in the code base, resulting in a massive PR with 1,379 lines changed, 311 of them deleted entirely. Deleting code feels good 😁.

Forem: Gabriela Voicu

Refactoring an Overgrown Notifications Class in Django

The Initial State

Phase 1

Phase 2