CSC/ECE 517 Fall 2013/oss E818 mra: Difference between revisions
Line 20: | Line 20: | ||
===What was done:=== | ===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. | 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 = | = Analysis = |
Revision as of 17:53, 30 October 2013
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. The Expertiza environment is already set up in NC State's VCL image "Ruby on Rails". If you have access, this is quickest way to get a development environment running for Expertiza. See the Expertiza wiki(provide hyperlink) on developing Expertiza on the VCL.
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
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 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:
- assignment_analytic.rb
- self.questionnaire_unique?
- questionnaire_of_type
- team_review_counts
- team_scores
- course_analytic.rb
- average_num_assignment_teams
- average_assignment_score
- assignment_review_counts
- assignment_team_counts
- assignment_average_scores
- assignment_max_scores
- assignment_min_scores
- assignment_team_analytic.rb
- average_review_word_count
- average_review_score
- average_review_character_count
- review_character_counts
- review_scores
- review_word_counts
- questionnaire_analytic.rb
- questions_text_list
- word_count_list
- character_count_list
- response_analytic.rb
- word_count_list
- character_count_list
- question_score_list
- comments_text_list
Post-refactoring report
Here is a summary of the results.
Future work
- 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.
- 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.