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.

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.

We have left a few comments in the controller that say 'used to be...' so that it can be seen where the changes have been made and these will be removed before the final submission.

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 create method was refactored so that the branch condition would be lower and there would be no repetition. As per the delegation design pattern we took a part of the functionality from the create method and put it in its own 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 functionality for this method was further delegated to two separate methods update_assignment_questionnaire_id and update_due_date_id. This way we don't have one method doing too many tasks at once. Now the create delegates some tasks to update_assignment_form and that method delegates to update_assignment_questionnaire_id and update_due_date_id.

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 as well as the new methods created.

Old create method:

New create method:

update_assignment_form 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.

Testing Details

RSpec

There were already quite a few test cases for the AssignmentsController. We have not added any new tests but made sure that no tests were failing due to our changes. 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