CSC/ECE 517 Fall 2013/oss E818 mra

E818 Refactoring and testing -- analytics

This Wiki page provides a detailed description of the Open Source Software project conducted on Expertiza, as part of the Object Oriented Languages and Systems coursework.

Introduction to Expertiza

Expertiza is an open source project built using the Ruby on Rails platform. It is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities to manage peer reviews in coursework. The source code can be forked from github and cloned for performing modifications.

Overview of project

Classes:

• analytic_controller.rb (163 lines)
• assignment_analytic.rb (206 lines)
• assignment_team_analytic.rb (126 lines)
• course_analytic.rb (142 lines)
• question_analytic.rb (12 lines)
• questionnaire_analytic.rb (55 lines)
• response_analytic.rb (101 lines)
• score_analytic.rb (12 lines)

What they do:

Display information about various system components in summary form, to aid in analysis.

What was done:

Analytic_controller.rb and course_analytic.rb had much duplicated code, which was be factored out. Additionally, analytic_controller.rb had methods that were too complicated, and were be separated into shorter, well-named methods, to make the operation of the class more transparent. Rest of the modules had duplicate code, which was refactored out.

Analysis

We used CodeClimate to analyse the existing code. As can been be seen, there are several classes with duplicate code, high complexity and code smells.

• High complexity - AnalyticController, CourseAnalytic, QuestionnaireAnalytic
• Duplication – AnalyticController, AssignmentAnalytic, AssignmentTeamAnalytic, CourseAnalytic, ResponseAnalytic
• Smells - AnalyticController, AssignmentAnalytic, AssignmentTeamAnalytic, CourseAnalytic, ResponseAnalytic

As regards complexity, along with a high overall complexity, the classes also have a high complexity per method ratio

Design Changes

Due to a lot of duplication of code that occurred in the AssignmentAnalytic, AssignmentTeamAnalytic and CourseAnalytic module, there was a definite need to refactor the duplicate code and extract out the duplication by moving the common trends in the code to a common module named as CommonAnalytic.

Here is the original design which has a lot of code duplication particularly in the modules AssignmentAnalytic, AssignmentTeamAnalytic and CourseAnalytic.

Thus the figure below gives the refactored design which removes the common and duplicate code from AssignmentAnalytic, AssignmentTeamAnalytic and CourseAnalytic module and moves them to a common module CommonAnalytic.

refactored expertiza design for analytic

In a similar way the modules ScoreAnalytic and QuestionAnalytic had redundant and duplicate code for getting word count, character count and unique character count. Thus again there was a need to refactor the duplicate code and extract out the duplication and move it a CountAnalytic module which would act as a center module thereby allowing ScoreAnalytic and QuestionAnalytic to delegate functions such as word count, character count and unique character count to the CountAnalytic module as shown below.

countQuestion

Code changes

Refactoring in AnalyticController

Usage of inbuilt methods.

def course_list
    courses = associated_courses(session[:user])
    course_list = Array.new
    courses.each do |course|
      course_list << [course.name, course.id]
    end
    respond_to do |format|
      format.json { render :json => sort_by_name(course_list) }
    end
  end

After refactoring

  def course_list
    courses = associated_courses(session[:user])
    course_list = courses.map { |course| [course.name, course.id] }
    render :json => sort_by_name(course_list)
  end

The above refactoring was applied to team_list and assignment_list as well.

Use of hashes

 case params[:type]
      when "line"
        chart_data = line_graph_data(params[:scope], params[:id], params[:data_type])
      when "bar"
        chart_data = bar_chart_data(params[:scope], params[:id], params[:data_type])
      when "scatter"
        chart_data = scatter_plot_data(params[:scope], params[:id], params[:data_type])
      when "pie"
        chart_data = pie_chart_data(params[:scope], params[:id], params[:data_type])
    end

Hash was used to adhere to the DRY principle.

    graph_method_name = {'line' => 'line_graph_data',
                         'bar' => 'bar_chart_data',
                         'scatter' => 'scatter_plot_data',
                         'pie' => 'pie_graph_data'}
    graph_data = send(graph_method_name[graph_type], scope, id, data_type)

Methods complexity

Complex methods were broken down into smaller methods to enhance readability.

Refactoring in Modules

Code movement

The modules were initially placed in app/models/ . They were moved to /lib/ folder

Code Changes

At many places the developers had added the logic to find, map, get unique object, etc. which already exist in Enumerable module. We changed the code to use these in-built methods. Some of the examples of such changes are:

1. Code before refactor [self.questionnaire_unique? in assignment_analytic.rb]

def self.questionnaire_unique?
    self.all.each do |assignment|
      assignment.questionnaire_types.each do |questionnaire_type|
        questionnaire_list = Array.new
        assignment.questionnaires.each do |questionnaire|
          if questionnaire.type == questionnaire_type
            questionnaire_list << questionnaire
          end
          if questionnaire_list.count > 1
            return false
          end
        end
      end
    end
    return true
  end

Code after refactoring. Here the inbuilt uniq method was used. If there are duplicates, then unique array length will be less than actual length.

def self.questionnaire_unique?
    self.all.each do |assignment|
      questionnaires = assignment.questionnaires
      return false if questionnaires.uniq { |q| q.type }.length < questionnaires.length
    end
    return true
end

2. Code before refactoring [ questionnaire_of_type(type_name_in_string) in assignment_analytic.rb ]

def questionnaire_of_type(type_name_in_string)
    self.questionnaires.each do |questionnaire|
      if questionnaire.type == type_name_in_string
        return questionnaire
      end
    end
end

Code after refactoring: The find method was used here.

def questionnaire_of_type(type_name_in_string)
    self.questionnaires.find { |questionnaire| questionnaire == type_name_in_string }
end

3 Code before refactoring [average_num_team_reviews in assignment_analytic ]. This pattern existed in many classes.

  def average_num_team_reviews
    if num_teams == 0
      0
    else
      total_num_team_reviews.to_f/num_teams
    end
  end

Code after refactoring:

  def average_num_team_reviews
    total_num_team_reviews.to_f/num_teams
  rescue ZeroDivisionError
    0
  end

The code consisted of several functions with unnecessary complexity. We refactored the code to reduce the complexity.
The code consisted of several functions that had duplicate code. These methods transformed a list of objects into a list of one of the object’s attributes. This pattern was removed by creating a generic function. Following was one of the pattern that existed:

# Expertiza code before refactoring in assignment_analytic.rb
def team_review_counts     
  list = Array.new     
  self.teams.each do |team|       
         list << team.num_reviews     
  end      
  if (list.empty?)       
          [0]     
  else      
          list
  end
end

The refactored code for the above logic:

 
# Helper function moved to common_analytic.rb
def extract_from_list(list, property)
      list.map { |item| item.send(property) }
end
 
# Method in assignment_analytic.rb
def team_review_counts     
      extract_from_list self.teams, :num_reviews
      (list.empty?) ? [0] : list    
end

The logic of the method was exactly what the map method does. Futher, a ternary if-else was used to enhance readability.

The above pattern accounted for majority of the duplicate code. Code before refactoring [self.types in QuestionnaireAnalytic ]

def self.types
      type_list = Array.new
      self.all.each do |questionnaire|
        if !type_list.include?(questionnaire.type)
          type_list << questionnaire.type
        end
      end
      type_list
end

Code after refactoring.

def self.types
    type_list = extract_from_list self.all, :type
    type_list.uniq
end

The following is a list of methods and classes changed for refactoring:

1. assignment_analytic.rb
   1. self.questionnaire_unique?
   2. questionnaire_of_type
   3. team_review_counts
   4. team_scores
2. course_analytic.rb
   1. average_num_assignment_teams
   2. average_assignment_score
   3. assignment_review_counts
   4. assignment_team_counts
   5. assignment_average_scores
   6. assignment_max_scores
   7. assignment_min_scores
3. assignment_team_analytic.rb
   1. average_review_word_count
   2. average_review_score
   3. average_review_character_count
   4. review_character_counts
   5. review_scores
   6. review_word_counts
4. questionnaire_analytic.rb
   1. questions_text_list
   2. word_count_list
   3. character_count_list
5. response_analytic.rb
   1. word_count_list
   2. character_count_list
   3. question_score_list
   4. comments_text_list

Post-refactoring report

Here is a summary of the results.

Future work

1. Currently only bar charts can be seen. Further work can be done to get different kinds of graphs such as pie chart, scatter plot, etc.
2. There are some requests that take a long time to respond. For example, View Scores Request. They can be optimized to reduce the response time.

External Links

1. http://guides.rubyonrails.org/ A guide for Ruby on Rails
2. http://c2.com/cgi/wiki?WhatIsRefactoring What is Refactoring?
3. https://codeclimate.com/ Automated Code Review
4. https://github.com/expertiza/expertiza
5. https://github.com/bbatsov/ruby-style-guide Ruby Style Guide