CSC/ECE 517 Spring 2020 E2009 Refactor assignment

Introduction

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.

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 many conditional branches 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 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.

Refactoring longer methods

The longer methods are refactored mostly using extract method. The methods refactored using Extract method are scores method and review_questionnaire_id method.

Refactor scores method

1) 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. 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

<\pre>

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 these changes the number of lines of code reduced to 17.

2) The review_questionnaire_id method

Refactoring for good coding practices
1. Removing unused variables

2. Changing variable names 

3. Avoiding multi-line ternary operators

4. Using Guard Clause instead o 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.

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.

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.

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.