CSC/ECE 517 Spring 2020 - E2004. Refactor assignments controller
This wiki page contains description of changes made as part of E2004, an OSS assignment for Spring 2020, CSC/ECE 517
Background
Expertiza allows the instructors and teaching assistants to store, retrieve, edit or delete assignments for a course that they are teaching. assignments_controller.rb is one of the biggest controllers that has a lot of scope for refactoring. It consists of the CRUD operations for an assignment along with small helper functions related to the deadlines, meta-reviews, team formation and so on. The primary issues with the code in this controller are listed as tasks.
Tasks
The branch condition size for create method is too high
Current implementation of create method has too many assignments and multiple layers of if conditions. It also has too many lines of code. Code Climate has identified following issues with the method:
Assignment Branch Condition size for create is too high. [56.37/15] Method `create` has 33 lines of code (exceeds 25 allowed). Consider refactoring.
We extracted a method called handle_assignment_form_save from the code to handle the case when assignment_form is saved. This allowed in reducing the number of lines in the create method. Similarly, a helper method called handle_assignment_form_save was extracted out of the code from handle_assignment_form_save to make sure each method was only doing a single task. In order to reduce too many conditional statements, we also separated out code to handle for non existent assignment directory as handle_assignment_directory_path_nonexist method.
By doing all of this, we were able to reduce the number of lines in the create method from 33 to 14, and Assignment Branch Condition size from 56.37 to less than 15.
The branch condition size for edit method is too high
Assignment Branch Condition size for edit is too high. [29.58/15]
This method had assignments and method calls that could be consolidated. By removing and grouping assignments and method calls into helper methods, we can still use them and reduce the branch condition size.
Thus, the methods update_due_date, update_assignment_badges, handle_missing_assignment_details, and user_timezone_specified were created.
- update_due_date contains the loop that adjusts due dates for an assignment.
- update_assignment_badges updates instance variables
- handle_missing_assignment_details handles some incomplete assignment details
- user_timezone_specified flashes an error if the user did not specify a preferred time zone.
The branch condition size for copy method is too high
Assignment Branch Condition size for copy is too high. [19.42/15]
The branch condition size for empty_rubrics_list method is too high
The current implementation of the empty_rubrics_list method has many different branches of the code that could be executed. Code Climate identified Assignment Branch Condition size to be too high.
Assignment Branch Condition size for empty_rubrics_list is too high. [18.47/15]
These include multiple if/unless statements as well as loops that may or may not be executed. We sought to reduce this problem by breaking up the sections of the code with different branches into their own functions, reducing the branch condition size for empty_rubrics_list.
We extracted code to remove questionnaire types from the rubric list that are already in existing assignment questionnaires into a method named remove_existing_questionnaire.
Likewise, we extracted code that conditionally removes questionnaire types from the rubric list (such as removing the teammate review questionnaire if the team only has a size of 1) into a method named remove_invalid_questionnaires.
The branch condition size for update_feedback_assignment_form_attributes method is too high
This implementation uses nested and sequential conditional statements. While it works to decide which flash message should appear, it can be simplified by removing the nested structure and using elsif statements.
The cognitive complexity of create method is too high
The current implementation of the create method has a high cognitive complexity because there are nested iteration blocks. Code Climate shows following issue:
Method `create` has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
There are two .each blocks, which are used to update assignment_questionnaire and due_date. We have resolved this issue by extracting those blocks into two separate methods named update_assignment_questionnaire and update_assignment_questionnaire.
The cognitive complexity of empty_rubrics_list method is too high
The current implementation of the empty_rubrics_list method has a high cognitive complexity due to there being nested iteration blocks.Code Climate indicates following issue:
Method `empty_rubrics_list` has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
There is a .each block on the assignment questionnaires and there is a .reject block on the rubric list entries nested within it. We sought to solve this issue by extracting the .reject block into a separate method named remove_existing_questionnaire.
A line in the handle_current_user_timezonepref_nil method is too long
The existing code had a line length of 180 characters.
Line is too long. [180/160]
Rubocop style guide recommends line length to be less than 100 characters. We have used backslash (\) to split the line into two lines of 98, and 97 characters each.
Testing
The assignments_controller.rb file has a testing file called the assignment_controller_sepc.rb. We ran rspec tests, and made sure all the existing tests passed with all our changes.
Here is a link to a video showing test results including rubocop results for assignments_controller.rb
Application Link: http://152.46.18.46:8080
Username: instructor6
Password: password
Future improvements
Team Information
- Dave Bell (dbell5)
- Steven Green (stgreen)
- Abhishek Upadhyaya Ghimire (aupadhy3)
- Mentor: Anuja Kulkarni