CSC/ECE 517 Spring 2019 E1910 Refactor assignments controller.rb

This wiki page is for the description of changes made under E1910 OSS assignment for Spring 2019, CSC/ECE 517.

We have not deployed this project since we did not change or add any functionality that would affect the user.

Expertiza Background

Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages. [3]

Description of the current project

The assignments_controller has multiple functions with wrong naming convention, they are classic examples of bad function names. Few of the variables names used in the method can also be refactored to make it more relevant and understandable. In addition, functions code can be optimized to ensure that it follows 'Don't Repeat Yourself' (DRY) principle. Also, most of the functions have missing code comments, which should be added to the functions.

Pull request link

[1]

Forked Github repo

[2]

Files modified in current project

The only controller modified for this project was the:
1. AssignmentsController
2. Assignment_helper

The updated method names in AssignmentController:

 - handle_assignment_directory_path_nonexist_case_and_answer_tagging 
   -- missing_submission_directory
 - assignment_form_key_nonexist_case_handler 
   -- assignment_submission_handler
 - handle_current_user_timezonepref_nil 
   -- timezone_handler
 - update_nil_dd_deadline_name 
   -- update_due_date_deadline_name
 - update_nil_dd_description_url 
   -- update_due_date_description_url

 - assignment_form_assignment_staggered_deadline? 
   -- assignment_staggered_deadline?
 - check_due_date_nameurl_not_empty 
   -- check_due_date_nameurl

 - update_feedback_assignment_form_attributes 
   -- update_feedback_attributes
 

The thought process behind changing the method names was to make the names shorter and reduce redundancy. There were a few rules that we learned from our mentor; words like nil and nonexist are not allowed in method names. So for handle_current_user_timezonepref_nil we first removed the nil. Then after looking at what the method does we determined that timezone_handler would be appropriate and tells the user enough at a moments glance. More information is available about the method in the form of a block comment.

Another example is assignment_form_assignment_staggered_deadline?, the assignment_form bit is redundant, since it is the assignments controller the beginning part does not provide any new information and assignment_staggered_deadline? is sufficient.

A different kind of naming is update_nil_dd_deadline_name, we removed the nil but the dd doesn't make sense. No one would know what that stands for just by reading it. This is why this method was changed to update_due_date_deadline_name. It is not any shorter but much more useful at a glance.

The assignment_helper file was added to. Some of the methods in the controller were misplaced and according to the DRY principle these methods are better suited to be in the helper method. The methods that were moved from the controller to the helper were:

 - assignment_submission_handler -> updated method name
 - check_due_date_nameurl(dd) -> updated method name

We were asked to refactor the action_allowed? method but after discussing some reafactoring methods with our mentor it was decided that refactoring this method would not promote the DRY principle and the methods created as a result would only be for that one purpose and therefore unnecessary.

Code Improvements

The DRY principle states, "Every piece of knowledge or logic must have a single, unambiguous representation within a system."[4] This project was about furthering this principle and keeping this controller concise and maintainable.

This is a controller that helps instructors, TA's, administrators, and super-administrators create/update/edit/show/delete current and past assignments. We updated the aforementioned method names so that the naming convention follows the DRY principle. Many of the method names were far too long, repetitive or unhelpful in understanding what the method actually does. In accordance with the principle we updated the method names to be simple while also being useful in letting the programmer know what the method does.

Many of these methods did not have any comments to help the programmer understand the logic driving the method. To further help the programmer in understanding what the code does we added block comments at the beginning of each method that we updated.

We cleaned up the controller by moving some methods from the controller into the associated helper file (assignment_helper.rb). By doing this we made the controller less messy and easier to read while maintaining and furthering the DRY principle. The code is now easily maintainable and all "helper" methods are in one place rather than separated across many files.

The delegation pattern states, "In delegation, an object handles a request by delegating to a second object (the delegate). The delegate is a helper object, but with the original context." [5]

As something extra, the create method was refactored so that the branch condition would be lower and there would be no repetition. We decided to use the delegation pattern because it serves the purpose of cleaning up existing methods while maintaining functionality. As per this pattern we took a request from the create method and put it in a helper method called update_assignment_form. This method handles updating the id of the assignment_questionnaire and the due_date and updating the form accordingly. The create method delegates this task to the update_assignment_form.

The update_assignment_form delegated a tasks to the helper method, array_traverser. This method does the job of going through the array and updates id fields to be strings instead of an integer.

As a result of these changes two code climate issues have been resolved which are stated below in the Code Climate section. Below are images of the way the create method used to look and how it looks now.

Old create method:

New create method:

Code Climate

Code Climate checks to make sure that the code that has been written follows certain rules so that everything is legible and concise. During our refactoring we have fixed the following code climate issues.

- The create method's Cognitive Complexity was too high. That has been resolved by the delegation we have described above.
- The create method was too long with 33 lines of code. That has been resolved by the delegation we have described above.
- The Branch Condition size for assignment_form_key_nonexist_case_handler, which is now assignment_submission_handler, was too high. This means that there were too many nested if/elsif/else statements. This has been resolved as a result of us refactoring the code.
- The Branch Condition size for update_feedback_assignment_form_attributes, which is now update_feedback_attributes
, was too high. This means that there were too many nested if/elsif/else statements. This has been resolved as a result of us refactoring the code.

We have three current code climate issues but after discussing with our mentor we have been told to ignore those.

Test Plan

The code coverage for the controller was 49% when we got the project. The code coverage when we finished was 53.32%.

There is a video on Expertiza titled "Rspec_test.mp4" which shows the updated tests running successfully.

RSpec Tests

There were already quite a few test cases for the AssignmentsController (assignments_controller_spec.rb). The create block tested just about everything. The two methods we added are private and since we branched out some functionality this needed to be a bit more explicitly tested. There is no precedence for testing private methods and it was frowned upon as per the video The Magic Tricks of Testing. We tested the public method, create, that calls the private method, update_assignment_form and array_traverser. We added a test with the context when assignment_form is saved successfully but fails to update.

For the other untested public methods, we found no usage directly in the controller, therefore, we did not test them. These methods are performing the tasks of assigning values to the variables and therefore that is being tested in other methods of the controllers.

No changes were made to the associated views and model file.

References

1. https://github.com/expertiza/expertiza/pull/1377
2. https://github.com/veehakhanna/expertiza
3. http://wiki.expertiza.ncsu.edu/index.php/CSC/ECE_517_Fall_2015/oss_E1553_AAJ
4. https://dzone.com/articles/software-design-principles-dry-and-kiss
5. https://en.wikipedia.org/wiki/Delegation_pattern