CSC/ECE 517 Spring 2020 E2009 Refactor assignment: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
 
(94 intermediate revisions by 2 users not shown)
Line 3: Line 3:
[http://wiki.expertiza.ncsu.edu/index.php/Expertiza_documentation 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.
[http://wiki.expertiza.ncsu.edu/index.php/Expertiza_documentation 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 ==
===== 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.
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. It 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.
 
[[File:Assignments.jpg]]
 
Below is screenshot of questionnaire on expertiza. It shows all the kinds of questionnaires we can create on expertiza.


== Refactoring assignment.rb ==
== Refactoring assignment.rb ==
Line 20: Line 16:
3) Large number of methods in one file.
3) Large number of methods in one file.


4) Dead code
4) No proper naming conventions in some places.
 
5) No proper naming conventions in some places.


== Approach ==
===== Approach =====
The approach we took to refactor this file, is to go through the issues generated by [https://codeclimate.com/github/expertiza/expertiza/app/models/assignment.rb 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.
The approach we took to refactor this file, is to go through the issues generated by [https://codeclimate.com/github/expertiza/expertiza/app/models/assignment.rb 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.
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
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.
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.
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.  


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


4) Perceived complexity - It is complexity score that's a measure of the complexity the reader experiences when looking at a method.
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.
All the metrics are quite interlinked, so aiming to reduce one complexity will also reduce other metrics.


== Refactoring longer methods ==
== 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.  
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 and response_map_to_metareview.  


=====Refactor scores 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
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.  


<pre>
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. Two methods are extracted out of this method. Below is the screenshot for diff after refactoring (left is refactored code)
  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.  
[[File:scores131313.png]]


'''After refactoring'''
<pre>
  #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
</pre>


Hence after refactoring the '''number of lines of code reduced to 17'''.
Hence after refactoring the '''number of lines of code in the method reduced to 17'''.


=====Refactor review_questionnaire_id method=====
=====Refactor review_questionnaire_id method=====


This method as the name implies returns the review questionnaire ID.  
This method as the name implies returns the review questionnaire ID. It first gets all the assignment questionnaire ids whose type is 'ReviewQuestionnaire'. This part is extracted as a separate method. Also renamed the variable rev_q_ids. Following is the diff after refactoring
'''Before refactoring'''
 
<pre>
[[File:s333.jpg]]
  def review_questionnaire_id(round = nil)
 
    # Get the round it's in from the next duedates
=====Refactor response_map_to_metareview method=====
    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
</pre>


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'''
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 place 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


<pre>
  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)
[[File:response_mappp.png]]
    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
</pre>


== Refactoring to reduce code complexity ==
== Refactoring to reduce code complexity ==


1) '''Reducing Cyclomatic Complexity''' <br />
1) '''Refactoring export_details_fields'''
Cyclomatic complexity is a software metric used to indicate the complexity of a program.  
The export_details_fields has many if statements as can be seen below. Each if statement increases the cyclomatic complexity by 1. It is refactored by adding constant EXPORT_DETAILS_FIELDS with all the fields to export. Then this list is iterated using each loop.
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.  
[[File:s100.jpg]]
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,
2) '''Refactoring delete method''' <br />
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.
The delete method has the following lines of code. Each line has a call to each method. This is also refactored by using list to hold all the instances to delete and then iterating through the list and deleting.
Loops can be said to have an exit condition, so they add one.
 
[[File:del.png]]
 
===== Reducing cyclomatic complexity and improving code reuse =====
 
1) Three methods in assignment.rb file has the following check


''' Before Refactoring '''
<pre>
  def self.export_details_fields(detail_options)   def self.export_details_fields(detail_options)
if self.staggered_deadline and topic_id.nil?
    fields = []     fields = []
</pre>
    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 '''
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.
  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


<pre>
def staggered_and_no_topic?(topic_id)
  self.staggered_deadline? and topic_id.nil?
end
</pre>


2) '''Reducing Perceived Complexity''' <br />
2) Similarly the method valid_num_review has if condition with 3 predicates as can be seen below
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 '''
''' 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 '''
<pre>
     DELETE_INSTANCES=['invitations','teams','participants','due_dates','assignment_questionnaires']
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")
</pre>


    DELETE_INSTANCES.each do |instance|
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
      self.instance_eval(instance).each(&:destroy)
    end


3) Cyclomatic complexity and code reuse.
''' After Refactoring '''
<pre>
#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
</pre>
<pre>
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
</pre>


Code climate gives current_stage_name and link_for_current_stage
By this way, it is both reducing cyclomatic complexity and improving code reuse.


== Refactoring for other good coding practices ==
== Refactoring for other good coding practices ==
Line 256: Line 130:


2. Changing variable/function names  
2. Changing variable/function names  


''' Before Refactoring '''
''' Before Refactoring '''
Line 265: Line 138:
     @courses = Assignment.assign_courses_to_assignment(current_user)
     @courses = Assignment.assign_courses_to_assignment(current_user)


3. Avoiding multi-line ternary operators
3. Avoiding multi-line ternary operators <br>
 
[[File:terter.png]]


4. '''Using Guard Clause instead of wrapping the code inside a conditional expression.''' <br />
 
4. Using Guard Clause instead of wrapping the code inside a conditional expression. <br />
A guard clause is simply a check that immediately exits the function, either with a  
A guard clause is simply a check that immediately exits the function, either with a  
return statement or an exception.
return statement or an exception.


'''Before Refactoring'''
[[File:guardclauseimg.jpg]]
  team[:scores] ?
 
        tcsv.push(team[:scores][:max], team[:scores][:min], team[:scores][:avg]) :
5. Converted simple if-else statements with ternary operator in some places.
        tcsv.push('---', '---', '---')
 
[[File:if2ternary.png]]
 
== Test Plan ==


'''After Refactoring'''
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. <br> Two rspec tests were failing before we started the refactoring. Those were fixed.
<pre>
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. Here is the link to the [https://www.youtube.com/watch?v=zSySTQp3t3w&feature=youtu.be tests] running.
if team[:scores]
        tcsv.push(team[:scores][:max], team[:scores][:min], team[:scores][:avg])
      else
        tcsv.push('---', '---', '---')
      end
</pre>
<pre>
unless self.staggered_deadline?
</pre>
<pre>
return due_date.deadline_name unless self.staggered_deadline?
</pre>


5. '''Reducing Cognitive Complexity''' <br />
[[File:s.png]]
Cyclomatic complexity is a static analysis measure of how difficult is code to test. Cognitive complexity tells us,
<br>
how difficult code is to understand by a reader.


We launched expertiza in browser and tested as thorough as possible. On testing the GUI's Assignment page completely, we found that "View Scores" is breaking only for "Project 1" due to a NoMethodError in the Grades#view. "View Scores" of all other projects work as expected. There was also a Routing Error for viewing the survey responses. These errors existed in the base code before we started the refactoring. Since these errors do not pertain to assignment.rb, they have not been fixed. Other than these errors everthing related to assignments works fine.


== Further refactoring possibilities ==


8. '''ABC Size''' <br />
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.
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.

Latest revision as of 21:53, 2 April 2020

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. It 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.

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 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. Two methods are extracted out of this method. Below is the screenshot for diff after refactoring (left is refactored code)


Hence after refactoring the number of lines of code in the method reduced to 17.

Refactor review_questionnaire_id method

This method as the name implies returns the review questionnaire ID. It first gets all the assignment questionnaire ids whose type is 'ReviewQuestionnaire'. This part is extracted as a separate method. Also renamed the variable rev_q_ids. Following is the diff after refactoring

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 place 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. It is refactored by adding constant EXPORT_DETAILS_FIELDS with all the fields to export. Then this list is iterated using each loop.

2) Refactoring delete method

The delete method has the following lines of code. Each line has a call to each method. This is also refactored by using list to hold all the instances to delete and then iterating through the list and deleting.

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.

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.
Two rspec tests were failing before we started the refactoring. Those were fixed. 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. Here is the link to the tests running.


We launched expertiza in browser and tested as thorough as possible. On testing the GUI's Assignment page completely, we found that "View Scores" is breaking only for "Project 1" due to a NoMethodError in the Grades#view. "View Scores" of all other projects work as expected. There was also a Routing Error for viewing the survey responses. These errors existed in the base code before we started the refactoring. Since these errors do not pertain to assignment.rb, they have not been fixed. Other than these errors everthing related to assignments works fine.

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.

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2020_E2009_Refactor_assignment&oldid=132674"