CSC/ECE 517 Fall 2016/E1659. Refactor on the fly calc.rb

This wiki page is for the description of changes made under E1659 OSS assignment for Fall 2016, CSC/ECE 517.

Peer Review Information

For users intending to view the deployed Expertiza associated with this assignment, the credentials are below:

• Instructor login: username -> instructor6, password -> password
• Student login: username -> student5431, password -> password
• Student login: username -> student5427, password -> password

Expertiza Background

Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NCSU. It’s an open source project developed on Ruby on Rails platform and it’s code is available on Github. Using Expertiza students can bid for a particular project topic and the faculty assigns the same to different groups. Students can track their grades for previous projects and make new submissions for current projects. It also allows students to review each other’s work and improve their work upon this feedback.

Description of the current project

We worked on a module named on_the_fly_calc in this Expertiza based OSS project. We focussed on refactoring some methods which were very complex and also modified the language of the module to make it more ruby-istic. We also worked on refactoring the code to reduce redundancy. Our goal in this project was to make this model easier to read and maintain.

Files modified in current project

A model was modified for this project namely:
1. On_the_fly_calc

On_the_fly_calc

On_the_fly_calc is a module that is included in assignment.rb. What this module does is that it calculates the score for both students and instructors. E.g., when a student clicks “view my score”, Expertiza calculates the review scores for each review. This is called “on the fly” because expertiza stores the review scores based on each question (in answers table), but does not store the total score that a reviewer gives to a reviewee. It directly calculates the total review score for each review, using the score values in the answer table during run-time. Eg: For a set of 4 questions a reviewer gives 5 on 5 points for 2 questions and 4 on 5 points for other 2 questions. So only these points are stored in the database and when a reviewee clicks to see the score given by this particular reviewer, on_the_fly_calc module helps calculate the total score i.e. 90% in this case.

List of changes

The main aim was to reduce the complexity grade of ‘on_the_fly_calc.rb’ module from “F” to “C” in CodeClimate, which was achieved. We worked on the following work items(WIs) in order to refactor the module:
WI1 : Refactor ‘Similar Code’ in lines 34-42 and 62-70 in on_the_fly_calc.rb
WI2 : Prefer ‘each’ over every instance of ‘for loop’
WI3 : Refactor ‘scores’ method to reduce ABC (assignment, branch and condition) size
WI4 : Refactor ‘compute_reviews_hash’ method to reduce ABC (assignment, branch and condition) size
WI5 : Refactor ‘compute_avg_and_ranges_hash’ method to reduce ABC (assignment, branch and condition) size

Solutions Implemented and Delivered

Refactored similar code

Lines 34-42 and 62-70 were similar in compute_reviews_hash method. Hence, a dedicated method ‘calc_review_score’ was defined and this method was called in the both instances inside compute_reviews_hash.

Lines 34-42 in compute_reviews_hash:

Similar code in Lines 62-70:

Refactored method 'calc_review_score' for the two similar codes:

Each loop prefered over For loop

In every instance where ‘for’ loops were used in on_the_fly_calc module, ‘each…do’ was replaced instead. This significantly reduced the Cyclomatic complexity of the code.

The following screen shows the instances where for loops where replaced:

Refactored methods - 'scores', 'compute_reviews_hash', 'compute_avg_and_ranges_hash'

1. a) Method ‘scores’ was very complex, performing too many functions within a single method. Hence, every specific function was broken down into 4 separate smaller methods. This significantly reduced the Assignment, Branch, Condition size for ‘scores’ method.

def scores(questions)
    scores = {}
    score_team = scores[:teams][index.to_s.to_sym]
    score = score_team[:scores]
    scores[:participants] = {}
    participant_score          #New method defined - participant_score
    scores[:teams] = {}
    index = 0
    self.teams.each do |team|
      score_team = {}
      score_team[:team] = team
      if self.varying_rubrics_by_round?
        assess                #New method defined - assess
        calculate_score       #New method defined - calculate_score
        calculate_assessment  #New method defined - calculate_assessment
      else
        assessments = ReviewResponseMap.get_assessments_for(team)
        score = Answer.compute_scores(assessments, questions[:review])
      end
      index += 1
    end
    scores
 end

b) The following 4 methods were created after splitting the first method
i. participant_score

def participant_score
  self.participants.each do |participant|
    scores[:participants][participant.id.to_s.to_sym] = participant.scores(questions)
  end
end

ii. assess

def assess
  self.num_review_rounds.each do |i|
    total_score = 0
    total_num_of_assessments = 0 # calculate grades for each rounds
    grades_by_rounds = {}
    round = grades_by_rounds[round_sym]
    assessments = ReviewResponseMap.get_assessments_round_for(team, i)
    round_sym = ("review" + i.to_s).to_sym
    round = Answer.compute_scores(assessments, questions[round_sym])
    total_num_of_assessments += assessments.size
    total_score += round[:avg] * assessments.size.to_f unless round[:avg].nil?
  end
end

iii. calculate_score

def calculate_score
  score = {}
  score[:max] = -999_999_999
  score[:min] = 999_999_999
  score[:avg] = 0
  grades_by_rounds = {}
  round = grades_by_rounds[round_sym]
  self.num_review_rounds.each do |i|
    round_sym = ("review" + i.to_s).to_sym
    grades_by_rounds = {}
    round = grades_by_rounds[round_sym]
    score[:max] = round[:max] if max_condition
    score[:min] = round[:min] if min_condition
  end
end

iv. calculate_assessment

def calculate_assessment
  if total_num_of_assessments.nonzero?
    score[:avg] = total_score / total_num_of_assessments
  else
    score[:avg] = nil
    score[:max] = 0
    score[:min] = 0
  end
end

c) Complex and long statements or loops were broken into simpler multiple statements.

For example, the unless condition in Lines 132-134 was refactored to Line 127. This reduced the Perceived Complexity of 'scores' method.

2. a) We also refactored method ‘compute_reviews_hash’ which was very complex and modularized individual functionalities into 2 separate methods. b) The following 2 methods were created and called after refactoring:
i. scores_varying_rubrics
ii. scores_non_varying_rubrics

3. Similarly, method ‘compute_avg_and_ranges_hash’ was also refactored and a seperate method calc_contri_score was defined to calculate contributor’s score. This method was then called from ‘compute_avg_and_ranges_hash’.

Testing Details

UI Testing

Login into Expertiza as a student or an instructor. Go to an assignment which has some reviews for a previous project and click on view scores. The total score for a review is calculated on the fly using this module. You will be able to see the average score that each reviewer gave you on the top right of your page. This ensured that the refactoring done by us didn't break the code and it delivered it's expected functionality.

Scope for future improvement

Every code has future improvements possible. We have evolved the Code Climate Grade 'F' to a Grade 'C' currently by removing the Cyclomatic complexity, the Perceived Complexity and replaced with the "for" with the 'each' to reduce the size . There are modifications that can still reduce the "ABC" Assignment Branches and Conditions Score as we can include the variables and functions in the "before action clauses", and rename long names with shorter yet appropriate names thus moving towards the score of "A".