CSC/ECE 517 Fall 2013/oss E818 mra: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
Line 19: Line 19:


# 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:
# 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:
Code before refactor
##Code before refactor [self.questionnaire_unique? in assignment_analytic.rb]
<pre>
<pre>
def self.questionnaire_unique?
def self.questionnaire_unique?
Line 47: Line 47:
     end
     end
     return true
     return true
end
</pre>
##Code before refactoring [ questionnaire_of_type(type_name_in_string) in assignment_analytic.rb ]
<pre>
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
</pre>
Code after refactoring. The find method was used here.
<pre>
def questionnaire_of_type(type_name_in_string)
    self.questionnaires.find { |questionnaire| questionnaire == type_name_in_string }
end
</pre>
Code before refactoring [average_num_team_reviews in assignment_analytic ]
<pre>
  def average_num_team_reviews
    if num_teams == 0
      0
    else
      total_num_team_reviews.to_f/num_teams
    end
  end
</pre>
Code after refactoring.
<pre>
  def average_num_team_reviews
    total_num_team_reviews.to_f/num_teams
  rescue ZeroDivisionError
    0
   end
   end
</pre>
</pre>
##self.questionnaire_unique? in assignment_analytic.rb
##questionnaire_of_type(type_name_in_string) in assignment_analytic.rb
#The code consisted of several functions with unnecessary complexity. We refactored the code to reduce the complexity.
#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.
#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.

Revision as of 16:46, 30 October 2013

Topic and short description (E818 Refactoring and testing -- analytics)

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 needs to be done:

At least analytic_controller.rb and course_analytic.rb have much duplicated code, which should be factored out. Additionally, analytic_controller.rb has methods that are too complicated, and should be separated into shorter, well-named methods, to make the operation of the class more transparent. Also, there are absolutely no tests for any of these methods.

Code changes

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


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

Code before refactoring [average_num_team_reviews in assignment_analytic ]

  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
  1. The code consisted of several functions with unnecessary complexity. We refactored the code to reduce the complexity.
  2. 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 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

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.