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 assignment_form_save_handler from the code to handle the case when assignment_form is saved. This allowed in reducing the number of lines in the create method from 33 to 15. Assignment Branch Condition size was also reduced from 56.37 to less than 15.

Following methods were extracted from assignment_form_save_handler:

• fix_assignment_missing_path - to handle non existent directory path
• update_assignment_form - to update assignment due dates, and questionnaire array in assignment form

The branch condition size for edit method is too high

Code Climate shows following issue:

  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

Code Climate shows following issue:

   Assignment Branch Condition size for copy is too high. [19.42/15]

The branch size of the copy method was inflated due to:

• Assigning (old_assign and new_assign) things that are only used once or not all
• Calling methods to assign values only used once

To reduce the branch size:

• Group assignments into a helper method which reduces the assignment and method call count

We created helper methods update_copy_session and check_same_directory?

• update_copy_session contains assignments unused in the copy method, but relevant to the program
• check_same_directory compares directories for two assignment IDs, consolidating the conditional used in the copy method.

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.

Test Plan

RSpect tests exist for the main methods used in the assignments controller:

• action_allowed?
  • when params action is edit or update
    • when the role name of current user is super admin or admin
    • when current user is the instructor of current assignment
    • when current user is the ta of the course which current assignment belongs to
    • when current user is a ta but not the ta of the course which current assignment belongs to
    • when current user is the instructor of the course which current assignment belongs to
    • when current user is an instructor but not the instructor of current course or current assignment
  • when params action is not edit and update
  • when the role current user is super admin/admin/instructor/ta
  • when the role current user is student
• new
  • creates a new AssignmentForm object and renders assignment#new page
• create
  • when assignment_form is saved successfully
  • when assignment_form is not saved
• edit
  • when assignment has staggered deadlines
  • As value errors should be covered in the ability to make assignments, the only test needed for this is when deadlines are staggered. A reminder for specifying rubrics is also tested.
• update
  • when params does not have key :assignment_form
    • when assignment is saved successfully
    • when assignment is not saved successfully
  • when params has key :assignment_form
    • when the timezone preference of current user is nil and assignment form updates attributes successfully
    • when the timezone preference of current user is not nil and assignment form updates attributes successfully
• show
  • renders assignments#show page
• copy
  • when new assignment id fetches successfully
  • when new assignment directory is same as old
  • when new assignment id does not fetch successfully
• delete
  • when assignment is deleted successfully
  • when assignment is not deleted successfully

These tests were included and not added to because they cover all the instances that the methods would be used under. Our additions to the code, helper methods, do not have tests as they are used in the main methods and do not need their own tests. Otherwise, there would be redundant testing. The helper methods did not change any functionality.

Additional testing occurs in assignment_creation_spec, but that tests the integration of the assignments controller with other features. These may fail , but changes in the assignments_controller may not be the cause. Our build fails, but the fix is proposed in another team's pull request, as they picked up where other developers left off for that code.

Testing

To test our changes, follow these instructions to set up the expertiza environment. If our repository produces errors during setup, verify if the issue is our repository or your setup by trying to setup the main expertiza repository.

In a terminal do the following:

• navigate to the experiza folder
• enter: rspec /app/controllers/assignments_controller
  • for RSpec tests
• enter: rubocop /app/controllers/assignments_controller
  • for Rubocop tests

The assignments_controller.rb file has a testing file called the assignment_controller_sepc.rb. We ran rspec tests, and made sure all the tests passed with our changes.

We also ran rubocop for assignments_controller.rb file, and were able to eliminate the issues reported for methods we worked on.

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

Coverage

As can be seen from Coverall, our changes have increased the coverage of assignments_controller.rb by 10.9%, from 30.5% up to 41.41%. While we did not make any changes to the tests, we did reduce the amount of testable code by reducing redundancy and extracting common code in the controller.

Future improvements

• There are other portions of the assignments_controller that can be refactored.
• There could be a collection of messages that are used in the application that can then be referenced in the code. This would have two benefits: allowing for a different message collection to be used for translations and also removing duplicate messages like are seen here:

 ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].name, "Assignment #{assignment_form.assignment.id} was deleted.", request)
 flash[:success] = 'The assignment was successfully deleted.'

• More thorough tests can be added to the assignments_controller spec to get the coverage closer to 100%.

Team Information

• Dave Bell (dbell5)
• Steven Green (stgreen)
• Abhishek Upadhyaya Ghimire (aupadhy3)
• Mentor: Anuja Kulkarni

References

Code Climate Issues