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, enabling students to submit their assignments, also aiding TA's and profs to access and grade the assignemnts 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) Dead code
5) 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. For methods, 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 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.
3) Cognitive complexity - 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 complexity score that's a measure of the complexity the reader experiences when looking at a method.
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 using extract method. Longer methods generally has higher Assignment Branch Size and higher complexity metrics. The methods refactored using Extract method are scores method and review_questionnaire_id method.
Refactor scores method
The scores method is one of the biggest methods in assignment.rb file. It computes and returns the scores of participants and teams as a hash. 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. Below is the code before refactoring
def scores(questions)
scores = {}
scores[:participants] = {}
self.participants.each do |participant|
scores[:participants][participant.id.to_s.to_sym] = participant.scores(questions)
end
scores[:teams] = {}
index = 0
self.teams.each do |team|
scores[:teams][index.to_s.to_sym] = {}
scores[:teams][index.to_s.to_sym][:team] = team
if self.varying_rubrics_by_round?
grades_by_rounds = {}
total_score = 0
total_num_of_assessments = 0 # calculate grades for each rounds
(1..self.num_review_rounds).each do |i|
assessments = ReviewResponseMap.get_responses_for_team_round(team, i)
round_sym = ("review" + i.to_s).to_sym
grades_by_rounds[round_sym] = Answer.compute_scores(assessments, questions[round_sym])
total_num_of_assessments += assessments.size
total_score += grades_by_rounds[round_sym][:avg] * assessments.size.to_f unless grades_by_rounds[round_sym][:avg].nil?
end
# merge the grades from multiple rounds
scores[:teams][index.to_s.to_sym][:scores] = {}
scores[:teams][index.to_s.to_sym][:scores][:max] = -999_999_999
scores[:teams][index.to_s.to_sym][:scores][:min] = 999_999_999
scores[:teams][index.to_s.to_sym][:scores][:avg] = 0
(1..self.num_review_rounds).each do |i|
round_sym = ("review" + i.to_s).to_sym
if !grades_by_rounds[round_sym][:max].nil? && scores[:teams][index.to_s.to_sym][:scores][:max] < grades_by_rounds[round_sym][:max]
scores[:teams][index.to_s.to_sym][:scores][:max] = grades_by_rounds[round_sym][:max]
end
if !grades_by_rounds[round_sym][:min].nil? && scores[:teams][index.to_s.to_sym][:scores][:min] > grades_by_rounds[round_sym][:min]
scores[:teams][index.to_s.to_sym][:scores][:min] = grades_by_rounds[round_sym][:min]
end
end
if total_num_of_assessments != 0
scores[:teams][index.to_s.to_sym][:scores][:avg] = total_score / total_num_of_assessments
else
scores[:teams][index.to_s.to_sym][:scores][:avg] = nil
scores[:teams][index.to_s.to_sym][:scores][:max] = 0
scores[:teams][index.to_s.to_sym][:scores][:min] = 0
end
else
assessments = ReviewResponseMap.get_assessments_for(team)
scores[:teams][index.to_s.to_sym][:scores] = Answer.compute_scores(assessments, questions[:review])
end
index += 1
end
scores
end
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 3 levels of nesting and many lines of code inside the outer each loop. When examined carefully the logic inside the if block performs 2 distinct task. The if condition is for the assignments with varying rubrics. Hence logic inside the if blocks is first it computes the grades from different rounds and then as second step it merges the grades from different rounds to get max, min and avg scores. So two methods are extracted out of this block : 1) compute_grades_by_rounds - computes and returns the grades from different rounds 2) merge_grades_by_rounds - merges the grades from different rounds as computed in step 1 to compute max, min and avg scores.
After refactoring
#Computes and returns the scores of assignment for participants and teams
def scores(questions)
scores = {:participants => {}, :teams => {}}
self.participants.each do |participant|
scores[:participants][participant.id.to_s.to_sym] = participant.scores(questions)
end
index = 0
self.teams.each do |team|
scores[:teams][index.to_s.to_sym] = {:team => team, :scores => {}}
if self.varying_rubrics_by_round?
grades_by_rounds, total_num_of_assessments, total_score = compute_grades_by_rounds(questions, team)
# merge the grades from multiple rounds
scores[:teams][index.to_s.to_sym][:scores] = merge_grades_by_rounds(grades_by_rounds, total_num_of_assessments, total_score)
else
assessments = ReviewResponseMap.get_assessments_for(team)
scores[:teams][index.to_s.to_sym][:scores] = Answer.compute_scores(assessments, questions[:review])
end
index += 1
end
scores
end
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. Before refactoring
def review_questionnaire_id(round = nil)
# Get the round it's in from the next duedates
if round.nil?
next_due_date = DueDate.get_next_due_date(self.id)
round = next_due_date.try(:round)
end
# for program 1 like assignment, if same rubric is used in both rounds,
# the 'used_in_round' field in 'assignment_questionnaires' will be null,
# since one field can only store one integer
# if rev_q_ids is empty, Expertiza will try to find questionnaire whose type is 'ReviewQuestionnaire'.
rev_q_ids = if round.nil?
AssignmentQuestionnaire.where(assignment_id: self.id)
else
AssignmentQuestionnaire.where(assignment_id: self.id, used_in_round: round)
end
if rev_q_ids.empty?
AssignmentQuestionnaire.where(assignment_id: self.id).find_each do |aq|
rev_q_ids << aq if aq.questionnaire.type == "ReviewQuestionnaire"
end
end
review_questionnaire_id = nil
rev_q_ids.each do |rqid|
next if rqid.questionnaire_id.nil?
rtype = Questionnaire.find(rqid.questionnaire_id).type
if rtype == 'ReviewQuestionnaire'
review_questionnaire_id = rqid.questionnaire_id
break
end
end
review_questionnaire_id
end
If a block of code inside a method is commented it is a good indication to extract that part of code as another method. In this method, such a part is computing rev_q_ids. (rev_q_ids is not following good naming convention, but probably review_questionnaire_id variable is already taken for the actual review_questionnaire_id, so developer would have given rev_q_ids). Hence after extracting get_rev_q_ids as separate method, below is the method after refactoring
def review_questionnaire_id(round = nil)
# Get the round it's in from the next duedates
if round.nil?
next_due_date = DueDate.get_next_due_date(self.id)
round = next_due_date.try(:round)
end
rev_q_ids = get_questionnaire_ids(round)
review_questionnaire_id = nil
rev_q_ids.each do |rqid|
next if rqid.questionnaire_id.nil?
rtype = Questionnaire.find(rqid.questionnaire_id).type
if rtype == 'ReviewQuestionnaire'
review_questionnaire_id = rqid.questionnaire_id
break
end
end
review_questionnaire_id
end
Refactoring to reduce code complexity
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
5. Reducing Cognitive Complexity
Cyclomatic complexity is a static analysis measure of how difficult is code to test. Cognitive complexity tells us,
how difficult code is to understand by a reader.
6. Reducing Cyclomatic Complexity
Cyclomatic complexity is a software metric used to indicate the complexity of a program.
It is a quantitative measure of the number of linearly independent paths through a program's source code.
This cop checks that the cyclomatic complexity of methods is not higher than the configured maximum.
The cyclomatic complexity is the number of linearly independent paths through a method.
The algorithm counts decision points and adds one. 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.
Before Refactoring
def self.export_details_fields(detail_options) def self.export_details_fields(detail_options) fields = [] fields = [] fields << 'Team ID / Author ID' if detail_options['team_id'] == 'true' EXPORT_FIELDS.each do |key, value| fields << 'Reviewee (Team / Student Name)' if detail_options['team_name'] == 'true' fields << value if detail_options[key]=='true' fields << 'Reviewer' if detail_options['reviewer'] == 'true' end fields << 'Question / Criterion' if detail_options['question'] == 'true' fields << 'Question ID' if detail_options['question_id'] == 'true' fields << 'Answer / Comment ID' if detail_options['comment_id'] == 'true' fields << 'Answer / Comment' if detail_options['comments'] == 'true' fields << 'Score' if detail_options['score'] == 'true' fields fields end
After Refactoring
EXPORT_FIELDS={'team_id'=>'Team ID / Author ID', 'team_name'=>'Reviewee (Team / Student Name)','reviewer'=>'Reviewer','question'=>'Question / Criterion','question_id'=>'Question ID','comment_id'=>'Answer / Comment ID','comments'=>'Answer / Comment','score'=>'Score' }.freeze
def self.export_details_fields(detail_options)
fields = []
EXPORT_FIELDS.each do |key, value|
fields << value if detail_options[key]=='true'
end
7. Reducing Perceived Complexity
This cop tries to produce a complexity score that's a measure of the complexity the reader experiences when looking
at a method. For that reason it considers `when` nodes as something that doesn't add as much complexity
as an `if` or a `&&`. Except if it's one of those special `case`/`when` constructs where there's no expression
after `case`. Then the cop treats it as an `if`/`elsif`/`elsif`… and lets all the `when` nodes count. In contrast
to the CyclomaticComplexity cop, this cop considers `else` nodes as adding complexity.
Before Refactoring
self.invitations.each(&:destroy) self.teams.each(&:delete) self.participants.each(&:delete) self.due_dates.each(&:destroy) self.assignment_questionnaires.each(&:destroy)
After Refactoring
DELETE_INSTANCES=['invitations','teams','participants','due_dates','assignment_questionnaires']
DELETE_INSTANCES.each do |instance|
self.instance_eval(instance).each(&:destroy)
end
8. ABC Size
This cop checks that the ABC size of methods is not higher than the configured maximum.
The ABC size is based on assignments, branches (method calls), and conditions.