CSC/ECE 517 Spring 2017 E1711: Difference between revisions
No edit summary |
No edit summary |
||
| Line 66: | Line 66: | ||
===Existing Tests=== | ===Existing Tests=== | ||
There were six existing Rspec tests for <i>delayed_mailer.rb</i>, but the existing tests only verify that a new delayed job was added to the queue based on the type of deadline specified when creating a new <code>DelayedMalier</code>. We deemed the existing testing as adequate for this section of the file as the <code>perform</code> method calls other methods to find the email addresses and send the email. | |||
===Our Testing=== | ===Our Testing=== | ||
We | We did not change existing testing for the <code>perform</code> method as the existing testing verified that a job to send out emails was added to the queue. We did, however, verify that our code changes did not break this portion of the code by continuously running the existing test cases. | ||
=Refactor <code>mail_signed_up_users</code>= | |||
Revision as of 01:37, 23 March 2017
E1711. Refactor delayed_mailer.rb and scheduled_task.rb
Overview
Introduction to Expertiza
Expertiza is a web based tool that allows instructors to create collaborative assignments where students can provide peer feedback on each others work. It provides instructors a system to manage assignments and different courses.
Scope of project E1711
The goal of E1711 was to refactor code found in the files delayed_mailer.rb and scheduled_task.rb.
The following issues were identified as the scope of this project:
- scheduled_task.rb duplicated most of its code from delayed_mailer.rb. Our objective is to reduce/remove the duplication in keeping with the DRY principle.
- The
performmethod uses a giant case statement, instead we were to incorporate the use of polymorphism - The
mail_signed_up_usersis long and should be broken into smaller and better named methods - Add/modify test cases as we added/removed/modified areas of the code
Information about the assignment can be found on this document
Test Plan
As this was a purely refactoring effort, our testing consisted of confirming existing tests did not break, modifying existing tests to match our changes, or adding tests as needed. The below sections will cover each objective in detail and will include information on how testing was done for each of those changes. See each of the Our Testing sections.
Remove duplicated code between delayed_mailer.rb and scheduled_task.rb
Background of Objective
The main objective of this task was to reduce duplicated code as recommended by DRY principle.
After a deep investigation into understanding why the same code would appear in two separate files, we concluded that the authors of scheduled_task.rb found that delayed_mailer.rb already integrated with a Ruby gem called delayed_job. delayed_job is used to run tasks asynchronously in the background. It was used in delayed_mailer to add an outgoing email to a queue to send out in the future. Instead of recreating similar infrastructure, the authors copied over the file to a different location, added on the new desired functionality, and renamed it. The authors also pointed all existing code to use scheduled_task.rb instead of delayed_mailer.rb
Our Changes
We found that the vast majority of the code in both the files applied to sending emails so we decided to merge the scheduled_task.rb into delayed_mailer.rb by porting the enhancements made for the scheduled tasks into delayed_mailer.rb. Doing so allowed us to meet the objective of DRYing out the code. However, we still have a situation where scheduled tasks feature has code in a section devoted to mailer code. We recommend that a future project take up the needed changes to pull out the scheduled task code out of the mailer file and make it a standalone feature as it should have been originally. See details in the Future Refactoring Opportunities below.
Existing Tests
We did not find any existing automated testing targeting this area of the code. All existing tests in the spec file associated with this feature were copies of the tests in the delayed mailer feature and did not exercise any of the new code that was introduced with adding the new types of scheduled tasks.
Our Testing
We went back to the demonstration of this feature and manually tested all the same scenarios. We verified that no functionality presented in the scheduled tasks demonstration was broken. Tests were added for the delayed mailer feature (covered in more details in the following sections). We recommend tests focused towards scheduled tasks are added once the scheduled tasks code is removed from the delayed mailer file back into an independent file.
Refactor the perform mehtod
Background of Objective
The objective of this task was to clean up this method. It was identified as being badly named, long, and not taking advantage of good design. It is essentially a giant case statement where one of the variables passed into the DelayedMailer constructor determines the people who receives the email.. Some suggested ways to refactor this function was first rename the perform to better describe what it does and to use polymorphism in place of the case statement. The original thought was that polymorphism could be used to determine what was sent out in each email based on the type of the email.
Our Changes
The first item we looked at was to rename the method. A generic name such as perform usually means that the method is doing too much or does not have a clear objective. Unfortunately, we found we could not rename the method because the delayed_job gem requires a method named perform in order to work on a custom job. See documentation here
The second part of the planned refactoring was to use polymorphism to determine the content of the email. After simplifying much of the code in the file, we discovered that the content of the email was independent of the variables used to initialize the object. At that point, we weighed the complexity of new code to use polymorphism versus a significantly more simplified case statement, and because of the lack of existing tests, decided that the simplified case statement was not only short, it is also very simple to follow the flow through the code. We instead focused on DRYing out this code as a better return on immediate investment as opposed to adding polymorphism.
Existing Tests
There were six existing Rspec tests for delayed_mailer.rb, but the existing tests only verify that a new delayed job was added to the queue based on the type of deadline specified when creating a new DelayedMalier. We deemed the existing testing as adequate for this section of the file as the perform method calls other methods to find the email addresses and send the email.
Our Testing
We did not change existing testing for the perform method as the existing testing verified that a job to send out emails was added to the queue. We did, however, verify that our code changes did not break this portion of the code by continuously running the existing test cases.
Refactor mail_signed_up_users
Background of Objective
After a deep investigation into understanding why the same code would appear in two separate files, we concluded that the authors of scheduled_task.rb found that delayed_mailer.rb already integrated with a Ruby gem called delayed_job. delayed_job is used to run tasks asynchronously in the background. It was used in delayed_mailer to add an outgoing email to a queue to send out in the future. Instead of recreating similar infrastructure, the authors copied over the file to a different location, added on the new desired functionality, and renamed it. The authors also pointed all existing code to use scheduled_task.rb instead of delayed_mailer.rb
Our Changes
We found that the vast majority of the code in both the files applied to sending emails so we decided to merge the scheduled_task.rb into delayed_mailer.rb by porting the enhancements made for the scheduled tasks into delayed_mailer.rb. Doing so allowed us to meet the objective of DRYing out the code. However, we still have a situation where scheduled tasks feature has code in a section devoted to mailer code. We recommend that a future project take up the needed changes to pull out the scheduled task code out of the mailer file and make it a standalone feature as it should have been originally. See details in the Future Refactoring Opportunities below.
Existing Tests
We did not find any existing automated testing targeting this area of the code. All existing tests in the spec file associated with this feature were copies of the tests in the delayed mailer feature and did not exercise any of the new code that was introduced with adding the new types of scheduled tasks.
Our Testing
We went back to the demonstration of this feature and manually tested all the same scenarios. We verified that no functionality presented in the scheduled tasks demonstration was broken. Tests were added for the delayed mailer feature (covered in more details in the following sections). We recommend tests focused towards scheduled tasks are added once the scheduled tasks code is removed from the delayed mailer file back into an independent file.
Quantitative Results from Refactoring Effort
We used Rubocop to show the improvements in that resulted from this effort.
Some highlights from analysis of the original delayed_mailer.rb file compared with after our changes are:
Future Refactoring Opportunities
Once we started to refactor the code, we found many additional opportunities to refactor the files that was beyond the planned scope. We note down some of the opportunities here and leave it to future projects to address.