CSC/ECE 517 Spring 2020 E2009 Refactor assignment
Introduction
Expertiza is an open source web application project based on Ruby on rails framework. Expertiza allows instructors to add assignments and students to upload their submissions. The assignment.rb model file consists of some basic CRUD operations along with some methods which help calculate scores and export details etc. The goal of the project is to refactor assignment.rb file to follow good coding practices.
About Assignments
Assignments is the most important base class in expertiza. It enables students to submit their assignments, also aiding TA's and profs to access and grade the assignments and also gives support for peer reviews. This model is used for all the backend operations and DB querying related to Assignments. Assignments can be submitted, reviewed by other peers and scores assigned and accessed, keeping in mind the deadline constraints too.
Below is screenshot of questionnaire on expertiza. It shows all the kinds of questionnaires we can create on expertiza.
Refactoring assignment.rb
Some of the coding issues with the assignment.rb file are
1) Methods performing more than one tasks, resulting in long methods
2) Methods with multiple conditions and loops resulting in increased cyclomatic and cognitive complexity
3) Large number of methods in one file.
4) No proper naming conventions in some places.
Approach
The approach we took to refactor this file, is to go through the issues generated by code climate and fix the smaller issues first. This gave us an idea about what the code is doing and gave us a head start to fix bigger issues. 69 issues were found on code climate and through this project, 30-35 issues have been resolved. Few of the issues that were resolved was detected by rubocop.
Code climate gives different metrics that indicates the code quality. Some of the metrics the code climate gives are
1) Assignment Branch Condition (ABC) size - It is computed by counting the number of assignments, branches and conditions in a section of code. Specifically ABC size = sqrt(A*A + B*B + C*C), where A - number of assignments, B - number of branches, C - number of conditions.
2) Cyclomatic complexity - It is a quantitative measure of the number of linearly independent paths through a program's source code (or a method). It gives a measure of how difficult a code is to test. Higher the number of branches in a method, higher the number of independent paths and hence higher the cyclomatic complexity.
An if statement (or unless or ?:) increases the complexity by one. An else branch does not, since it doesn't add a decision point. The && operator (or keyword 'and') can be converted to a nested if statement, and ||/or is shorthand for a sequence of ifs, so they also add one. Loops can be said to have an exit condition, so they add one.
3) Cognitive complexity - It is a measure of how difficult a unit of code is to intuitively understand. Generally methods with higher cyclomatic complexity will have higher cognitive complexity also.
4) Perceived complexity - It is a complexity score that's a measure of the complexity the reader experiences when looking at a method. In contrast to the Cyclomatic Complexity, this metric considers `else` nodes as adding complexity.
All the metrics are quite interlinked, so aiming to reduce one complexity will also reduce other metrics.
Refactoring longer methods
The longer methods are refactored mostly by extracting a method out which performs an independent task. Longer methods generally has higher Assignment Branch Size and higher complexity metrics. The methods refactored using Extract method are scores, review_questionnaire_id, delete and response_map_to_metareview.
Refactor scores method
The scores method is one of the biggest methods in assignment.rb file. The code climate gives the Assignment Branch Condition size as 131.2/15, number of lines of code as 48 and cyclomatic complexity of 8/6.
The scores method computes and returns the scores of participants and teams as a hash. The participant scores are directly got by calling a method in Participant class. The logic to compute the scores of teams has an outer each loop with one if-else block inside and 2 each loops inside the if block. This resulted in 3 levels of nesting and many lines of code inside the each loop.
Hence after refactoring the number of lines of code reduced to 17.
Refactor review_questionnaire_id method
This method as the name implies returns the review questionnaire ID.
Refactor response_map_to_metareview method
This method returns the best review to meta-review map if meta-review exists for a review. It has a sequence of similar operations like sorting the review_respone_map and rejecting certain entries in the review_reponse_map based on certain criteria. We found the same lines repeated in two places, so we extracted that part as one method. In one the method includes a logic to get the reviewers in a map. Even though that task is performed in one place, it is performing a distinct task. Hence that part is also extracted out as a method. Following is the diff after refactoring
Refactoring to reduce code complexity
1) Refactoring export_details_fields The export_details_fields has many if statements as can be seen below. Each if statement increases the cyclomatic complexity by 1.
2) Refactoring delete method
The delete method has the following lines of code. Each line has call to each method.
Before Refactoring
self.invitations.each(&:destroy) self.teams.each(&:delete) self.participants.each(&:delete) self.due_dates.each(&:destroy) self.assignment_questionnaires.each(&:destroy)
This can be refactored as below
After Refactoring
DELETE_INSTANCES=['invitations','teams','participants','due_dates','assignment_questionnaires']
DELETE_INSTANCES.each do |instance|
self.instance_eval(instance).each(&:destroy)
end
Reducing cyclomatic complexity and improving code reuse
1) Three methods in assignment.rb file has the following check
if self.staggered_deadline and topic_id.nil?
Each predicate in if condition count towards one decision point and hence increase in cyclomatic complexity. Also the same condition check is done in 3 to 4 places. Hence we can write a separate method like below and call in all places the conditions self.staggered_deadline and topic_id.nil? are checked together.
def staggered_and_no_topic?(topic_id) self.staggered_deadline? and topic_id.nil? end
2) Similarly the method valid_num_review has if condition with 3 predicates as can be seen below
Before Refactoring
if self.num_reviews_allowed && self.num_reviews_allowed != -1 && self.num_reviews_allowed < self.num_reviews_required
self.errors.add(:message, "Num of reviews required cannot be greater than number of reviews allowed")
elsif self.num_metareviews_allowed && self.num_metareviews_allowed != -1 && self.num_metareviews_allowed < self.num_metareviews_required
self.errors.add(:message, "Number of Meta-Reviews required cannot be greater than number of meta-reviews allowed")
Here the 2 if conditions are quite similar. Also both are checking 3 conditions. Hence the condition check part can written as a separate function as below
After Refactoring
#returns true if reviews required is greater than reviews allowed
def num_reviews_greater?(reviews_required, reviews_allowed)
reviews_allowed && reviews_allowed != -1 && reviews_required > reviews_allowed
end
if num_reviews_greater?(self.num_reviews_required, self.num_reviews_allowed) self.errors.add(:message, "Num of reviews required cannot be greater than number of reviews allowed") elsif num_reviews_greater?(self.num_metareviews_required, self.num_metareviews_allowed) self.errors.add(:message, "Number of Meta-Reviews required cannot be greater than number of meta-reviews allowed") end
By this way, it is both reducing cyclomatic complexity and improving code reuse.
Refactoring for other good coding practices
1. Removing unused variables
2. Changing variable/function names
Before Refactoring
if @map.assignment.has_badge? @courses = Assignment.set_courses_to_assignment(current_user)
After Refactoring
if @map.assignment.badge? @courses = Assignment.assign_courses_to_assignment(current_user)
3. Avoiding multi-line ternary operators
4. Using Guard Clause instead of wrapping the code inside a conditional expression.
A guard clause is simply a check that immediately exits the function, either with a
return statement or an exception.
Before Refactoring
team[:scores] ?
tcsv.push(team[:scores][:max], team[:scores][:min], team[:scores][:avg]) :
tcsv.push('---', '---', '---')
After Refactoring
if team[:scores]
tcsv.push(team[:scores][:max], team[:scores][:min], team[:scores][:avg])
else
tcsv.push('---', '---', '---')
end
unless self.staggered_deadline?
return due_date.deadline_name unless self.staggered_deadline?
5. Converted simple if-else statements with ternary operator in some places.
Test Plan
Our test plan includes rspec tests and testing the GUI in the browser. The rspec tests for the assignment.rb file is pretty much complete with tests written for all the methods called from outside the file. The methods added as part of refactoring are given a private scope and are added in assignment.rb since it is not called from outside. Testing the public methods implicitly tests these private methods. Below is a screenshot our rspec test results.
Further refactoring possibilities
The assignment.rb file has too many methods in one file. It maybe a better idea to move some of the methods which are only used with the views to assignment_helper.rb file.