CSC/ECE 517 Spring 2015/oss E1505 xzl: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
 
(32 intermediate revisions by the same user not shown)
Line 1: Line 1:
==Expertiza - Refactoring AssignmentParticipant model==
==Expertiza - Refactoring AssignmentParticipant model==
Expertiza is a web application developed using Ruby on Rails that serves as a peer-review system. The application allows students to submit and peer-review learning objects (articles, code, web sites, etc)<ref>[https://github.com/expertiza/expertiza Expertiza on GitHub]</ref><ref>[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki Page]</ref>. It is an open source project and it's codebase is maintained in GitHub. We are contributing to Expertiza as a part of our Object-Oriented Design and Development's Open-Source Software (OSS) Project. Our goal in this project is to refactor the <b>StudentQuiz controller</b>. In this Wiki Page, we would be explaining the changes that we have made for the same.
Expertiza is a web application developed using Ruby on Rails that serves as a peer-review system. The application allows students to submit and peer-review learning objects (articles, code, web sites, etc)<ref>[https://github.com/expertiza/expertiza Expertiza on GitHub]</ref><ref>[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki Page]</ref>. It is an open source project and it's codebase is maintained in GitHub. We are contributing to Expertiza as a part of our Object-Oriented Design and Development's Open-Source Software (OSS) Project. Our goal in this project is to refactor the <b>assignment_participant model</b>. In this Wiki Page, we will explain the changes that we have made for the same.
__TOC__
__TOC__


==Project Description==
==Project Description==
The <b>AssignmentParticipant model</b> is subclass of Participant model and is used to maintain the list of students/users participating in a given assignment.
The <b>assignment_participant model</b> is subclass of Participant model and is used to maintain the list of students/users participating in a given assignment.
<br>The changes that are needed to be done are described as follows:<ref>[https://docs.google.com/a/ncsu.edu/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit# GoogleDoc for our project requirements]</ref>
<br>The changes that are needed to be done are described as follows:<ref>[https://docs.google.com/a/ncsu.edu/document/d/1J0WUBtYV_MDhpEQ-50z8gXE-OrvVkpEaZxvn_9RCAsM/edit# GoogleDoc for our project requirements]</ref>
<ol>
<ol>
<li>Pluralize the class. (StudentQuizzesController)</li>
<li>Rename methods prefixed with “get”.</li>
<li>Rename methods to conform to RESTful style. (e.g. :- Rename the list method to index.)</li>
<li>Move methods to appropriate file helper classes.</li>
<li>Reduce the number of instance variables per controller action.</li>
<li>Move methods to appropriate models.</li>
<li>Review Method <b>graded?</b> for boolean zen.</li>
<li>Remove unused methods</li>
<li>Perform Code cleanup by removing unused code.</li>
<li>Use good Ruby style guidelines in the code.</li>
<li>Use good Ruby style guidelines in the code.</li>
<li>Split a method performing multiple tasks, into seperate methods.</li>
<li>Fix logical errors in the code.</li>
</ol>
</ol>


==Modifications made to the Existing Code==
==Modifications made to the Existing Code==


===Pluralized the class name StudentQuizController===
===Rename methods prefixed with “get”===
----
----
As per the Rails convention the controller class names are suggested to be plural.
According to Ruby naming conventions, the name of getter and setter methods need not to be verbs and they should not be prefixed with "get". <br>
This helps in generating RESTful routing URI helpers. Also, naming the classes as plural seems intuitive.<ref>[http://stackoverflow.com/questions/646951/singular-or-plural-controller-and-helper-names-in-rails Using the Singular or Plural controller and helper names in Rails]</ref>.  
The following methods were renamed.
We used the refactor functionality in RubyMine to rename the <b>StudentQuiz</b> controller class to <b>StudentQuizzes</b>.
 
The following files were modified and/or renamed.
<pre>
<pre>
app/controllers/review_mapping_controller.rb
get_reviewers => reviewers
app/controllers/{student_quiz_controller.rb → student_quizzes_controller.rb}
get_scores(questions) => scores(questions)
app/helpers/student_quiz_helper.rb
get_members => members
app/helpers/student_quizzes_helper.rb
get_team_hyperlinks => team_hyper_links
app/views/questionnaires/view.html.erb
get_hyperlinks_array => hyperlinks_array
app/views/{student_quiz → student_quizzes}/_quiz_form.html.erb
get_course_string => course_string
app/views/{student_quiz → student_quizzes}/_responses.html.erb
get_feedback => feedback
app/views/{student_quiz → student_quizzes}/_set_dynamic_quiz.html.erb
get_reviews => reviews
app/views/{student_quiz → student_quizzes}/_set_self_quiz.html.erb
self.get_export_fields(options) => self.export_fields(options)
app/views/{student_quiz → student_quizzes}/finished_quiz.html.erb
get_path => path
app/views/{student_quiz → student_quizzes}/grade_essays.html.erb
get_current_stage => current_stage
app/views/{student_quiz → student_quizzes}/list.html.erb
get_stage_deadline => stage_deadline
app/views/{student_quiz → student_quizzes}/take_quiz.html.erb
app/views/student_task/view.html.erb
app/views/tree_display/actions/_assignments_actions.html.erb
test/functional/{student_quiz_controller_test.rb → student_quizzes_controller_test.rb}
test/test_helper.rb
test/unit/helpers/student_quiz_helper_test.rb
test/unit/helpers/student_quizzes_helper_test.rb
</pre>
</pre>


The detailed change-set for this refactor can be seen [https://github.com/ankit3005/expertiza/commit/20981333a14a3f468f76cd6d414b3088975b71bd here].
===Move methods to appropriate file helper classes===
 
===RESTful style implementation===
----
----
The purpose of the <b>list</b> method in the <b>StudentQuizzes controller</b> is to list all the quizzes that are available to a particular user for a particular assignment. RESTful guidelines state that a method that returns a list of all available objects, in this case the quizzes, should be named as <b>index</b>. Therefore, we renamed the list method to the index method in the controller and in all the files that had references to the list method of the student_quizzes_controller (For e.g. :- The view finished_quiz.html.erb of the Student Quiz controller, the review_mapping controller.rb etc.).
Methods get_submitted_files, get_files should not be in this class. They deal with files and should be moved to appropriate file helper classes.<br>
 
[[File:FileHelper.png]]
<b>Before Refactoring :</b>
===Move methods to appropriate models===
<br> Method : list
<pre>
def list
    @participant = AssignmentParticipant.find(params[:id])
    return unless current_user_id?(@participant.user_id)
 
    @assignment = Assignment.find(@participant.parent_id)
 
    # Find the current phase that the assignment is in.
    @quiz_phase = @assignment.get_current_stage(AssignmentParticipant.find(params[:id]).topic_id)
 
    @quiz_mappings = QuizResponseMap.where(reviewer_id: @participant.id)
 
    # Calculate the number of quizzes that the user has completed so far.
    @num_quizzes_total = @quiz_mappings.size
 
    @num_quizzes_completed = 0
    @quiz_mappings.each do |map|
      @num_quizzes_completed += 1 if map.response
    end
 
    if @assignment.staggered_deadline?
      @quiz_mappings.each { |quiz_mapping|
        if @assignment.team_assignment?
          participant = AssignmentTeam.get_first_member(quiz_mapping.reviewee_id)
        else
          participant = quiz_mapping.reviewee
        end
 
        if !participant.nil? and !participant.topic_id.nil?
          quiz_due_date = TopicDeadline.where(topic_id: participant.topic_id, deadline_type_id: 1).first
        end
      }
      deadline_type_id = DeadlineType.find_by_name('quiz').id
    end
  end
}
</pre>
 
<b>After Refactoring :</b>
<br> Method : index
<pre>
def index
    participant = AssignmentParticipant.find(params[:id])
    return unless current_user_id?(participant.user_id)
    @assignment = Assignment.find(participant.parent_id)
    @quiz_mappings = QuizResponseMap.get_mappings_for_reviewer(participant.id)
end
</pre>
 
===Instance Variable Reductions===
----
----
In Rails, the data is shared between the controllers and views through the instance variables. The instance variabIes that are set during the execution a particular controller method are accessible to the view. Excessive usage of this standard method of data sharing between the controller and the view results in increased coupling between them <ref>[http://blog.remarkablelabs.com/2013/01/how-to-decrease-coupling-in-your-controllers-views-with-decent_exposure-for-better-maintainability Decreasing Controller-View Coupling in Rails]</ref>. Increased coupling results in several problems, including less maintainability of code, difficulty in code reuse etc. <ref>[http://en.wikipedia.org/wiki/Coupling_(computer_programming)#Disadvantages Disadvantages of Tight Coupling - Wikipedia]</ref> Thus, we need to reduce coupling as much as possible and in this case it can be achieved by reducing as many instance variables as possible in the controller. Therefore, we have refactored our code in order to eliminate unnecessary instance variables and to convert all the instance variables to local variables that are not used in the views.
reviewed_by? , quiz_taken_by? do not belong to AssignmentParticipant model. Move them to appropriate models. Because in the reviewed_by? method the keyword ParticipantReviewResponseMap is referenced, so we move the method reviewed_by? to model app/models/participant_review_response_map.rb, also the same to quiz_taken_by?, since  the keyword QuizQuestionnaire is referenced in method quiz_taken_by?, so we move it to model app/models/quiz_questionnaire.rb. Also We did the same operation to method get_quizzes_taken to move it to app/models/quiz_response_map.rb. <br>
 
Below we use a pair picture of method reviewed_by? as an example to demonstrate operation of this category.
In the <b>take_quiz method</b>, the following instance variables were removed:
[[File:Screen Shot 2015-03-21 at 4.14.06 PM.png]]
<br>
[[File:file_move.png]]
* Changed the <b>quizzes</b> instance variable to a local variable.
===Remove unused methods===
* Eliminated the instance variable <b>assignment</b> which was not being used anywhere.
* Eliminated the local variable <b>teams</b> which was not being used any where
 
<b>Before Refactoring</b>
<pre>
def self.take_quiz assignment_id , reviewer_id
  @quizzes = Array.new
  reviewer = Participant.where(user_id: reviewer_id, parent_id: assignment_id).first
  @assignment = Assignment.find(assignment_id)
  teams = TeamsUser.where(user_id: reviewer_id)
  Team.where(parent_id: assignment_id).each do |quiz_creator|
    unless TeamsUser.find_by_team_id(quiz_creator.id).user_id == reviewer_id
      Questionnaire.where(instructor_id: quiz_creator.id).each do |questionnaire|
        if !@assignment.team_assignment?
          unless QuizResponseMap.where(reviewed_object_id: questionnaire.id, reviewer_id:  reviewer.id).first
            @quizzes.push(questionnaire)
          end
        else unless QuizResponseMap.where(reviewed_object_id: questionnaire.id, reviewer_id:  reviewer_id).first
              @quizzes.push(questionnaire)
            end
        end
      end
    end
  end
  return @quizzes
end
</pre>
 
<b>After Refactoring</b>
<pre>
def self.take_quiz assignment_id , reviewer_id
    quizzes = Array.new
    reviewer = Participant.where(user_id: reviewer_id, parent_id: assignment_id).first
    Team.where(parent_id: assignment_id).each do |quiz_creator|
      unless TeamsUser.find_by_team_id(quiz_creator.id).user_id == reviewer_id
        Questionnaire.where(instructor_id: quiz_creator.id).each do |questionnaire|
          unless QuizResponseMap.where(reviewed_object_id: questionnaire.id, reviewer_id:  reviewer.id).first
            quizzes.push(questionnaire)
          end
        end
      end
    end
    return quizzes
  end
</pre>
 
===Follow principle of Boolean Zen===
----
----
In cases where a method only returns a boolean value by evaluating an expression (using the if..else construct), the if..else statement is redundant and can be eliminated. We have a method <b> graded? </b> that does this thing. Thus, we eliminated the if..else construct in the graded? method as it was redundant.<ref>[https://www.cs.utexas.edu/~scottm/cs312/handouts/slides/topic16_boolean_logic.pdf About Boolean Zen]</ref>
Methods from below are not getting invoked from anywhere, so they should be removed from this model. <br>
 
The following method are removed:
<b> Before Refactoring </b>
<br>
Method : graded?
<pre>
<pre>
def graded?(response, question)
average_score_per_assignment(assignment_id)
  if Score.where(question_id: question.id, response_id:  response.id).first
quiz_taken_by?(contributor, reviewer)
    return true
has_quiz?
  else
reviewees
    return false
get_two_node_cycles
  end
get_three_node_cycles
end
get_four_node_cycles
get_cycle_similarity_score(cycle)
get_cycle_deviation_score(cycle)
get_reviews_by_reviewer(reviewer)
get_submitted_files()
get_files(directory)
get_wiki_submissions
get_hash
review_response_maps
get_topic_string
</pre>
</pre>
 
===Use good Ruby style guidelines in the code===
<b> After Refactoring </b>
<br> Method : graded?
<pre>
def graded?(response, question)
  return (Score.where(question_id: question.id, response_id:  response.id).first)
end
</pre>
 
===Code Cleanup===
----
----
At some places, we found certain statements and variable assignments that were not being used later on in the method or code. We have eliminated such unused statements, so that methods contain only code that is being used later on. This would improve the readability and the maintainability of the code. For example in the code below we eliminated the <b>@quiz_phase</b> and the <b>@num_quizzes_total</b> variables because they were not being used anywhere in the code. Also, since the <b>local</b> variables <b>quiz_due_date, deadline_type_id, participant</b> were not being used anywhere, we could eliminate the entire <b>if</b> block from the code below. We then converted the instance variable <b>participant</b> to a local variable because it was not being used in the corresponding view. This is illustrated well in the example mentioned above for the [http://wiki.expertiza.ncsu.edu/index.php/CSC/ECE_517_Fall_2014/oss_E1460_aua#RESTful_style_implementation RESTful style implementation].
At many places in the code we found that the Ruby Style Guidelines were not followed. We have refactored the code so that it uses the good code style guidelines. The following are some of the refactorings the we have done:<ref>https://docs.google.com/a/ncsu.edu/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit</ref>
 
===Following Ruby Style Guidelines (Global Rules)===
----
At many places in the code we found that the Ruby Style Guidelines were not followed. We have refactored the code so that it uses the good code style guidelines. The following are some of the refactorings the we have done:-
<br>
<br>
==== Used .eql? instead of "==" ====
==== Use `&&` and `||` rather than `and` and `or` to keep boolean precedence  ====
----
----
<b>Before Refactoring</b>
<b>Before Refactoring</b>
<br> Method : finished_quiz


<pre> if score.score == -1 </pre>
<pre>  
 
line 596: if self.user.handle == nil or self.user.handle == ""
<b>After Refactoring</b>
line 624: if member.directory_num == nil or member.directory_num < 0
 
<pre> if score.score.eql? -1 </pre>
 
==== Eliminated the "== true/false" check ====
----
<b>Before Refactoring</b>
<br> Method : finished_quiz
 
<pre> if essay_not_graded == true </pre>
 
<b>After Refactoring</b>
 
<pre> if essay_not_graded </pre>
 
==== Use `&&` and `||` rather than `and` and `or` to keep boolean precedence ====
----
<b>Before Refactoring</b>
<br> Method: record_response
<pre> elsif  correct_answer and params["#{question.id}"] == correct_answer.txt </pre>
 
<b>After Refactoring</b>
<br> Method: calculate_score
<pre> elsif  correct_answer && params["#{question.id}"]== correct_answer.txt </pre>
 
==== Use key: ‘value’, not :key => ‘value’ ====
----
<b>Before Refactoring</b>
<br> Method: record_response
<pre>new_score = Score.new :comments => choice, :question_id => question.id, :response_id => @response.id</pre>
 
<b>After Refactoring</b>
<br> Method: calculate_score
<pre>new_score = Score.new comments: choice, question_id: question.id, response_id: response.id</pre>
 
==== Replace find_by_...  with a where command ====
----
<b> Before Refactoring </b>
<br> Method: record_response
<pre> if (QuestionType.find_by_question_id question.id).q_type == 'MCC' </pre>
 
<b> After Refactoring </b>
<br> Method: calculate_score
<pre>ques_type = (QuestionType.where( question_id: question.id)).q_type
if ques_type.eql? 'MCC'
</pre>
 
====Used .nil? instead of "== nil" ====
----
<b>Before Refactoring</b>
<br> Method: record_response
<pre>
if params["#{question.id}"] == nil
</pre>
</pre>


<b>After Refactoring</b>
<b>After Refactoring</b>
<br> Method: calculate_score
<pre>  
<pre>
line 404: if self.user.handle == nil || self.user.handle == ""
if params["#{question.id}"].nil?
line 432: if member.directory_num == nil || member.directory_num < 0
</pre>
</pre>


==== Used a Boolean variable when that is sufficient====
==== @users_team.size == 0 should be @users_team.zero? ====
----
----
<b>Before Refactoring</b>
<b>Before Refactoring</b>
<br> Method: record_response
<pre>
valid = 1
if valid == 0
</pre>


<b>After Refactoring</b>
<pre>  
<br> Method: calculate_score
line 45: return 0 if number_of_scores == 0
<pre>
line 55: return 0 if self.response_maps.size == 0
valid = false
line 294: if length_of_assessments=scores[round_sym][:assessments].nil? || scores[round_sym][:assessments].length==0
if valid
line 420: if course.name.strip.length == 0
</pre>
line 538: if all({conditions: ['user_id=? && parent_id=?', user.id, id]}).size == 0
 
====Use good conditional statements====
----
Using unless is a good practice. But it is not a good practice to use unless and ! within the condition. Instead we could use an if condition itself.
 
<b>Before Refactoring</b>
<br> Method: record_response
<pre>
unless new_score.comments != "" && new_score.comments
  valid = false
</pre>
</pre>


<b>After Refactoring</b>
<b>After Refactoring</b>
<br> Method: calculate_score
<pre>  
<pre>
line 45: return 0 if number_of_scores.zero?
if new_score.comments.empty? || new_score.comments.nil?
line 55: return 0 if self.response_maps.size.zero?
  valid = false
line 178: if length_of_assessments=scores[round_sym][:assessments].nil? || scores[round_sym][:assessments].length.zero?
line 304: if course.name.strip.length.zero?
line 355: if all({conditions: ['user_id=? && parent_id=?', user.id, id]}).size.zero?
</pre>
</pre>


====Use of Routing Helpers====
==== Use 'key:value' rather than ':key=>value'====
----
----
Routing helpers are a simpler alternative to the otherwise complex hard coded URLs which reduce the readability of the code. Routing helpers allow us to declare possible common routes for a given controller. Routing helpers have been implemented since they maintain consistency even if changes are made to the routing paths.
<b>Before Refactoring</b>:
<br> config.rb does not contain a student_quizzes resource
<br> review_mapping_controller.rb
<pre>
redirect_to :controller => 'student_quizzes', :action => 'index', :id => params[:participant_id]
</pre>
<b>After Refactoring</b>
<br> config.rb
<pre>
resources :student_quizzes, :only => [:index]
</pre>
review_mapping_controller.rb
<pre>
redirect_to student_quizzes_path(:id => params[:participant_id])
</pre>
===Replace controller method with a model method ===
----
Rails 4 conventions dictate the use of a fat model and skinny controller.
It is better to place the search function in the model rather than placing it in the controller.
The search code belonged to the <b>quiz_response_map</b> model, since it queries that particular table in the database.
The code was extracted into the <b>get_mappings_for_reviewer</b> method and placed in the file <b>quiz_response_map.rb</b>. This method was then called from the <b>StudentQuizzes</b> controller.
<ref name = "stackoverflow">[http://stackoverflow.com/questions/14044681/fat-models-and-skinny-controllers-sounds-like-creating-god-models Fat Models and Skinny Controllers.]</ref>
<b>Before Refactoring</b>
<br> student_quizzes_controller.rb
<br> Method : index
<pre>
@quiz_mappings = QuizResponseMap.where(reviewer_id: participant.id)
</pre>
<b>After Refactoring</b>
<br> student_quizzes_controller.rb
<br> Method : index
<pre>
@quiz_mappings = QuizResponseMap.get_mappings_for_reviewer(participant.id)
</pre>
<br> quiz_response_map.rb
<pre>
def self.get_mappings_for_reviewer(participant_id)
  return QuizResponseMap.where(reviewer_id: participant_id)
end
</pre>
===Changes made in method logic===
----
We have made certain changes in the logic of the methods calculate_score and record_response (previously the code of both these methods was only in record_response) primarily to improve the existing logic and eliminate redundant code. These changes are described as follows:
* The <b>score</b> variable was already being set to 0 on the loop entry, therefore it was redundant to reset score to zero again. Thus, we eliminated this line of code inside the if statement.
<b>Before Refactoring</b>
<pre>
questions.each do |question|
  score = 0
  if (QuestionType.find_by_question_id question.id).q_type == 'MCC'
    score = 0
</pre>
<b>After Refactoring</b>
<pre>
questions.each do |question|
score = 0
if ques_type.eql? 'MCC'
    # Eliminated score = 0 over here
</pre>
----
* The variable <b>correct_answer</b> stored multiple values as the where condition to which it was assigned was returning multiple values. Therefore it seemed more intuitive to rename <b>correct_answer</b> to <b>correct_answers</b> so that it is apparent that it contains multiple values.
<b>Before Refactoring</b>
<pre>
correct_answer = QuizQuestionChoice.where(question_id: question.id, iscorrect: 1)
</pre>
<b>After Refactoring</b>
<pre>
correct_answers = QuizQuestionChoice.where(question_id: question.id, iscorrect: true)
</pre>
----
* The below piece of code found out the question type twice in the same loop. Therefore we extracted it and assigned it to a local variable so that the query is executed only once.
<b>Before Refactoring</b>
<b>Before Refactoring</b>
<pre>
# First Query
if (QuestionType.find_by_question_id question.id).q_type == 'MCC'
# Repetition of the query in the same loop
if (QuestionType.find_by_question_id question.id).q_type == 'Essay'
</pre>
<b>After Refactoring</b>
<pre>
# Querying only once and assigning it to a local variable ques_type
ques_type = (QuestionType.where( question_id: question.id)).q_type
# Usage 1 of ques_type
if ques_type.eql? 'MCC'
# Usage 2 of ques_type
if ques_type.eql? 'Essay'
</pre>
----
* The <b>new_scores</b> and <b>scores</b> array stored almost the similar values. The <b>scores</b> array contained a copy of the value that the <b>new_scores</b> array contained. Therefore we eliminated the <b>new_scores</b> array and are performing all the operations only on the <b>scores</b> array.
<b>Before Refactoring</b>
<pre>
# Part 1
new_score = Score.new :comments => choice, :question_id => question.id, :response_id => @response.id
unless new_score.valid?
valid = 1
end
new_scores.push(new_score)
# Part 2  
new_scores.each do |score_update|
score_update.score = score
scores.push(score_update)
end
</pre>
<b>After Refactoring</b>
<pre>
# Part 1
new_score = Score.new comments: choice, question_id: question.id, response_id: response.id


unless new_score.valid?
<pre>
   valid = false
line 14:  belongs_to  :assignment, :class_name => 'Assignment', :foreign_key => 'parent_id'
end
line 15:  has_many    :review_mappings, :class_name => 'ParticipantReviewResponseMap', :foreign_key => 'reviewee_id'
scores.push(new_score)
line 16:  has_many    :quiz_mappings, :class_name => 'QuizResponseMap', :foreign_key => 'reviewee_id'
line 87: ParticipantReviewResponseMap.create(:reviewee_id => self.id, :reviewer_id => reviewer.id,
# Part 2
line 88:  :reviewed_object_id => assignment.id)
scores.each do |score_update|
line 95:   QuizResponseMap.create(:reviewed_object_id => quiz.id,:reviewee_id => contributor.id, :reviewer_id => reviewer.id,
  score_update.score = score
line 96:  :type=>"QuizResponseMap", :notification_accepted => 0)
end
line 100:  return AssignmentParticipant.where(:user_id=>user_id,:parent_id=>assignment_id).first
line 413:CourseParticipant.create(:user_id => self.user_id, :parent_id => course_id) if part.nil?
line 539:  new_part = AssignmentParticipant.create(:user_id => user.id, :parent_id => id)
line 612: new_submit = ResubmissionTime.new(:resubmitted_at => Time.now.to_s)
line 618:max_num = AssignmentParticipant.where(['parent_id = ?', self.parent_id], :order => 'directory_num desc').first.directory_num
</pre>
</pre>


----
* The logic to compute the final score for a multiple-choice, multiple-correct type of question seemed to be incorrect and therefore we fixed it.
<b>Before Refactoring</b>
<pre>
# Part 1 of the Scoring Logic
questions.each do |question|
score = 0
if (QuestionType.find_by_question_id question.id).q_type == 'MCC'
  score = 0
  if params["#{question.id}"] == nil
  valid = 1
  else
    correct_answer = QuizQuestionChoice.where(question_id: question.id, iscorrect: 1)
    params["#{question.id}"].each do |choice|
    correct_answer.each do |correct|
    if choice == correct.txt
      score += 1
    end
# Part 2 of the scoring logic which seems to award full points even if you marked some extra options apart from marking all correct answers
  unless score == correct_answer.count
  score = 0
  else
  score = 1
  end
</pre>
<b>After Refactoring</b>
<b>After Refactoring</b>
<pre>
<pre>
# Part 1 of the Scoring Logic
line 14: belongs_to  :assignment, class_name: 'Assignment', foreign_key: 'parent_id'
questions.each do |question|
line 15: has_many    :review_mappings, class_name: 'ParticipantReviewResponseMap', foreign_key: 'reviewee_id'
score = 0
line 16: has_many    :quiz_mappings, class_name: 'QuizResponseMap', foreign_key: 'reviewee_id'
correct_answers = QuizQuestionChoice.where(question_id: question.id, iscorrect: true)
line 74: ParticipantReviewResponseMap.create(reviewee_id: self.id, reviewer_id: reviewer.id,
ques_type = (QuestionType.where( question_id: question.id)).q_type
line 75: reviewed_object_id: assignment.id)
  if ques_type.eql? 'MCC'
line 82: QuizResponseMap.create(reviewed_object_id: quiz.id,reviewee_id: contributor.id, reviewer_id: reviewer.id,
    if params["#{question.id}"].nil?
line 83: type:"QuizResponseMap", notification_accepted: 0)
      valid = false
line 87: return AssignmentParticipant.where(user_id:user_id,parent_id:assignment_id).first
    else
line 297: CourseParticipant.create(user_id: self.user_id, parent_id: course_id) if part.nil?
        params["#{question.id}"].each do |choice|
line 356: new_part = AssignmentParticipant.create(user_id: user.id, parent_id: id)
          correct_answers.each do |correct|
line 420: new_submit = ResubmissionTime.new(resubmitted_at: Time.now.to_s)
          if choice.eql? correct.txt
line 426: max_num = AssignmentParticipant.where(['parent_id = ?', self.parent_id], order: 'directory_num desc').first.directory_num
            score += 1
          end
 
# Part 2 of the scoring logic - We have also compared the number of options the user selected to the total number of correct answers
if score.eql? correct_answers.count && score == params["#{question.id}"].count
  score = 1
else
  score = 0
end
</pre>
</pre>


==== Use good array checking ====
----
----
* The <b>record_response</b> function was performing two distinct operations : One operation was saving the response to the database and the other was to calculate the score for the questions. We created a new function <b>calculate_score</b> that would calculate the score for the questions and <b>record_response</b> now only performs the task of saving responses to the database. By doing this, we followed the <b>Single Responsibility O-O Design Principle for methods</b> mentioned by <b> Mr. Robert C. Martin </b> which states that "Every software module should have only one reason to change"<ref>[http://www.codeproject.com/Articles/567768/Object-Oriented-Design-Principles O-O Design Principles]</ref>
<b>Before Refactoring</b>
<b>Before Refactoring</b>
<pre>
def record_response
  @map = ResponseMap.find(params[:map_id])
  @response = Response.new()
  @response.map_id = params[:map_id]
  @response.created_at = DateTime.current
  @response.updated_at = DateTime.current
  @response.save


  @questionnaire = Questionnaire.find(@map.reviewed_object_id)
<pre>
  scores = Array.new
line 264: if(round!=nil)  
  new_scores = Array.new
line 301: if(scores[round_sym][:scores][:max]!=nil && scores[review_sym][:scores][:max]<scores[round_sym][:scores][:max])  
  valid = 0
line 304: if(scores[round_sym][:scores][:min]!= nil && scores[review_sym][:scores][:min]>scores[round_sym][:scores][:min])
  questions = Question.where(questionnaire_id: @questionnaire.id)
line 307: if(scores[round_sym][:scores][:avg]!=nil)
  questions.each do |question|
line 598: elsif AssignmentParticipant.where(parent_id: self.assignment.id, handle: self.user.handle).length > 0
    score = 0
    if (QuestionType.find_by_question_id question.id).q_type == 'MCC'
      score = 0
      if params["#{question.id}"] == nil
        valid = 1
      else
        correct_answer = QuizQuestionChoice.where(question_id: question.id, iscorrect: 1)
        params["#{question.id}"].each do |choice|
 
          correct_answer.each do |correct|
            if choice == correct.txt
              score += 1
            end
 
          end
          new_score = Score.new :comments => choice, :question_id => question.id, :response_id => @response.id
 
          unless new_score.valid?
            valid = 1
          end
          new_scores.push(new_score)
 
        end
        unless score == correct_answer.count
          score = 0
        else
          score = 1
        end
        new_scores.each do |score_update|
          score_update.score = score
          scores.push(score_update)
        end
      end
    else
      score = 0
      correct_answer = QuizQuestionChoice.where(question_id: question.id, iscorrect:  1).first
      if (QuestionType.find_by_question_id question.id).q_type == 'Essay'
        score = -1
      elsif  correct_answer and params["#{question.id}"] == correct_answer.txt
        score = 1
      end
      new_score = Score.new :comments => params["#{question.id}"], :question_id => question.id, :response_id => @response.id, :score => score
      unless new_score.comments != "" && new_score.comments
        valid = 1
      end
      scores.push(new_score)
    end
  end
  if valid == 0
    scores.each do |score|
      score.save
    end
    redirect_to :controller => 'student_quizzes', :action => 'finished_quiz', :map_id => @map.id
  else
    flash[:error] = "Please answer every question."
    redirect_to :action => :take_quiz, :assignment_id => params[:assignment_id], :questionnaire_id => @questionnaire.id
  end
 
end
</pre>
</pre>


<b>After Refactoring</b>
<b>After Refactoring</b>
<pre>
<pre>
# New record_response method
line 148: if(!round.nil?)
  def record_response
line 185: if(!scores[round_sym][:scores][:max].nil? && scores[review_sym][:scores][:max]>scores[round_sym][:scores][:max])
    map = ResponseMap.find(params[:map_id])
line 188: if(!scores[round_sym][:scores][:min].nil? && scores[review_sym][:scores][:min]>scores[round_sym][:scores][:min])
    response = Response.new
line 191: if(!scores[round_sym][:scores][:avg].nil?)
    response.map_id = params[:map_id]
line 406: elsif AssignmentParticipant.where(parent_id: self.assignment.id, handle: self.user.handle).length.any?
    response.created_at = DateTime.current
    response.updated_at = DateTime.current
    response.save
 
    calculate_score map,response
 
  end
 
# New calculate_score
def calculate_score map, response
    questionnaire = Questionnaire.find(map.reviewed_object_id)
    scores = Array.new
    valid = true
    questions = Question.where(questionnaire_id: questionnaire.id)
    questions.each do |question|
      score = 0
      correct_answers = QuizQuestionChoice.where(question_id: question.id, iscorrect: true)
      ques_type = (QuestionType.where( question_id: question.id)).q_type
      if ques_type.eql? 'MCC'
        if params["#{question.id}"].nil?
          valid = false
        else
          params["#{question.id}"].each do |choice|
 
            correct_answers.each do |correct|
              if choice.eql? correct.txt
                score += 1
              end
 
            end
            new_score = Score.new comments: choice, question_id: question.id, response_id: response.id
 
            unless new_score.valid?
              valid = false
            end
            scores.push(new_score)
 
          end
          if score.eql? correct_answers.count && score == params["#{question.id}"].count
            score = 1
          else
            score = 0
          end
          scores.each do |score_update|
            score_update.score = score
          end
        end
      else
        correct_answer = correct_answers.first
        if ques_type.eql? 'Essay'
          score = -1
        elsif  correct_answer && params["#{question.id}"]== correct_answer.txt
          score = 1
        end
        new_score = Score.new :comments => params["#{question.id}"], :question_id => question.id, :response_id => response.id, :score => score
        if new_score.comments.empty? || new_score.comments.nil?
          valid = false
        end
        scores.push(new_score)
      end
    end
    if valid
      scores.each do |score|
        score.save
      end
      redirect_to :controller => 'student_quizzes', :action => 'finished_quiz', :map_id => map.id
    else
      flash[:error] = "Please answer every question."
      redirect_to :action => :take_quiz, :assignment_id => params[:assignment_id], :questionnaire_id => questionnaire.id
    end
  end
</pre>
</pre>


The detailed change-set for this refactor can be seen here <ref>[https://github.com/z-Xie/expertiza/commit/c0d696476616f3f1ea429bda7eb8b52ccd10cdb2?diff=unified our github commit]</ref>.
==References==
==References==
<references/><br>
<references/><br>

Latest revision as of 21:40, 23 March 2015

Expertiza - Refactoring AssignmentParticipant model

Expertiza is a web application developed using Ruby on Rails that serves as a peer-review system. The application allows students to submit and peer-review learning objects (articles, code, web sites, etc)<ref>Expertiza on GitHub</ref><ref>Expertiza Wiki Page</ref>. It is an open source project and it's codebase is maintained in GitHub. We are contributing to Expertiza as a part of our Object-Oriented Design and Development's Open-Source Software (OSS) Project. Our goal in this project is to refactor the assignment_participant model. In this Wiki Page, we will explain the changes that we have made for the same.

Project Description

The assignment_participant model is subclass of Participant model and is used to maintain the list of students/users participating in a given assignment.
The changes that are needed to be done are described as follows:<ref>GoogleDoc for our project requirements</ref>

  1. Rename methods prefixed with “get”.
  2. Move methods to appropriate file helper classes.
  3. Move methods to appropriate models.
  4. Remove unused methods
  5. Use good Ruby style guidelines in the code.

Modifications made to the Existing Code

Rename methods prefixed with “get”


According to Ruby naming conventions, the name of getter and setter methods need not to be verbs and they should not be prefixed with "get".
The following methods were renamed.

get_reviewers => reviewers
get_scores(questions) => scores(questions)
get_members => members
get_team_hyperlinks => team_hyper_links
get_hyperlinks_array => hyperlinks_array
get_course_string => course_string
get_feedback => feedback
get_reviews => reviews
self.get_export_fields(options) => self.export_fields(options)
get_path => path
get_current_stage => current_stage
get_stage_deadline => stage_deadline

Move methods to appropriate file helper classes


Methods get_submitted_files, get_files should not be in this class. They deal with files and should be moved to appropriate file helper classes.

Move methods to appropriate models


reviewed_by? , quiz_taken_by? do not belong to AssignmentParticipant model. Move them to appropriate models. Because in the reviewed_by? method the keyword ParticipantReviewResponseMap is referenced, so we move the method reviewed_by? to model app/models/participant_review_response_map.rb, also the same to quiz_taken_by?, since the keyword QuizQuestionnaire is referenced in method quiz_taken_by?, so we move it to model app/models/quiz_questionnaire.rb. Also We did the same operation to method get_quizzes_taken to move it to app/models/quiz_response_map.rb.
Below we use a pair picture of method reviewed_by? as an example to demonstrate operation of this category.

Remove unused methods


Methods from below are not getting invoked from anywhere, so they should be removed from this model.
The following method are removed:

average_score_per_assignment(assignment_id)
quiz_taken_by?(contributor, reviewer)
has_quiz?
reviewees
get_two_node_cycles
get_three_node_cycles
get_four_node_cycles
get_cycle_similarity_score(cycle)
get_cycle_deviation_score(cycle)
get_reviews_by_reviewer(reviewer)
get_submitted_files()
get_files(directory)
get_wiki_submissions
get_hash
review_response_maps
get_topic_string

Use good Ruby style guidelines in the code


At many places in the code we found that the Ruby Style Guidelines were not followed. We have refactored the code so that it uses the good code style guidelines. The following are some of the refactorings the we have done:<ref>https://docs.google.com/a/ncsu.edu/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit</ref>

Use `&&` and `||` rather than `and` and `or` to keep boolean precedence


Before Refactoring

 
line 596: if self.user.handle == nil or self.user.handle == ""
line 624: if member.directory_num == nil or member.directory_num < 0

After Refactoring

 
line 404: if self.user.handle == nil || self.user.handle == ""
line 432: if member.directory_num == nil || member.directory_num < 0

@users_team.size == 0 should be @users_team.zero?


Before Refactoring

 
line 45: return 0 if number_of_scores == 0
line 55: return 0 if self.response_maps.size == 0
line 294: if length_of_assessments=scores[round_sym][:assessments].nil? || scores[round_sym][:assessments].length==0
line 420: if course.name.strip.length == 0
line 538: if all({conditions: ['user_id=? && parent_id=?', user.id, id]}).size == 0

After Refactoring

 
line 45: return 0 if number_of_scores.zero?
line 55: return 0 if self.response_maps.size.zero?
line 178: if length_of_assessments=scores[round_sym][:assessments].nil? || scores[round_sym][:assessments].length.zero?
line 304: if course.name.strip.length.zero?
line 355: if all({conditions: ['user_id=? && parent_id=?', user.id, id]}).size.zero?

Use 'key:value' rather than ':key=>value'


Before Refactoring

 
line 14:  belongs_to  :assignment, :class_name => 'Assignment', :foreign_key => 'parent_id'
line 15:  has_many    :review_mappings, :class_name => 'ParticipantReviewResponseMap', :foreign_key => 'reviewee_id'
line 16:  has_many    :quiz_mappings, :class_name => 'QuizResponseMap', :foreign_key => 'reviewee_id'
line 87: ParticipantReviewResponseMap.create(:reviewee_id => self.id, :reviewer_id => reviewer.id,
line 88:   :reviewed_object_id => assignment.id)
line 95:   QuizResponseMap.create(:reviewed_object_id => quiz.id,:reviewee_id => contributor.id, :reviewer_id => reviewer.id,
line 96:   :type=>"QuizResponseMap", :notification_accepted => 0)
line 100:  return AssignmentParticipant.where(:user_id=>user_id,:parent_id=>assignment_id).first
line 413:CourseParticipant.create(:user_id => self.user_id, :parent_id => course_id) if part.nil?
line 539:  new_part = AssignmentParticipant.create(:user_id => user.id, :parent_id => id)
line 612: new_submit = ResubmissionTime.new(:resubmitted_at => Time.now.to_s)
line 618:max_num = AssignmentParticipant.where(['parent_id = ?', self.parent_id], :order => 'directory_num desc').first.directory_num

After Refactoring

line 14: belongs_to  :assignment, class_name: 'Assignment', foreign_key: 'parent_id'
line 15: has_many    :review_mappings, class_name: 'ParticipantReviewResponseMap', foreign_key: 'reviewee_id'
line 16: has_many    :quiz_mappings, class_name: 'QuizResponseMap', foreign_key: 'reviewee_id' 
line 74: ParticipantReviewResponseMap.create(reviewee_id: self.id, reviewer_id: reviewer.id,
line 75: reviewed_object_id: assignment.id)
line 82: QuizResponseMap.create(reviewed_object_id: quiz.id,reviewee_id: contributor.id, reviewer_id: reviewer.id,
line 83: type:"QuizResponseMap", notification_accepted: 0)
line 87: return AssignmentParticipant.where(user_id:user_id,parent_id:assignment_id).first
line 297: CourseParticipant.create(user_id: self.user_id, parent_id: course_id) if part.nil?
line 356: new_part = AssignmentParticipant.create(user_id: user.id, parent_id: id)
line 420: new_submit = ResubmissionTime.new(resubmitted_at: Time.now.to_s)
line 426: max_num = AssignmentParticipant.where(['parent_id = ?', self.parent_id], order: 'directory_num desc').first.directory_num

Use good array checking


Before Refactoring

 
line 264: if(round!=nil) 
line 301: if(scores[round_sym][:scores][:max]!=nil && scores[review_sym][:scores][:max]<scores[round_sym][:scores][:max]) 
line 304: if(scores[round_sym][:scores][:min]!= nil && scores[review_sym][:scores][:min]>scores[round_sym][:scores][:min])
line 307: if(scores[round_sym][:scores][:avg]!=nil)
line 598: elsif AssignmentParticipant.where(parent_id: self.assignment.id, handle: self.user.handle).length > 0

After Refactoring

line 148: if(!round.nil?)
line 185: if(!scores[round_sym][:scores][:max].nil? && scores[review_sym][:scores][:max]>scores[round_sym][:scores][:max])
line 188: if(!scores[round_sym][:scores][:min].nil? && scores[review_sym][:scores][:min]>scores[round_sym][:scores][:min])
line 191: if(!scores[round_sym][:scores][:avg].nil?)
line 406: elsif AssignmentParticipant.where(parent_id: self.assignment.id, handle: self.user.handle).length.any?

The detailed change-set for this refactor can be seen here <ref>our github commit</ref>.

References

<references/>