CSC/ECE 517 Fall 2014/oss E1460 aua: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
 
(156 intermediate revisions by 3 users not shown)
Line 1: Line 1:
=Expertiza - Refactoring StudentQuizController=
==Expertiza - Refactoring StudentQuizController==
 
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.
__TOC__
__TOC__
==Introduction==
Expertiza<ref name="expertiza>''Expertiza'' http://wikis.lib.ncsu.edu/index.php/Expertiza</ref> is a web application available to both students and professors. The Expertiza project is a system to create reusable learning objects through peer review. Expertiza supports team projects, and the submission of almost any document type, including URLs and wiki pages. Students can keep a track of their assignments, teammates and can conduct peer reviews on diverse topics and projects. It is an open source project developed on Ruby on Rails platform. More information on Expertiza can be found [https://github.com/expertiza/expertiza here]. The source code can be forked and cloned for making modifications.
ertiza with main focus of Refactoring the Users Controller.


==Project Description==
==Project Description==
The <b>StudentQuiz controller</b> consists of methods involved in creating, scoring & recording responses of the quizzes taken by reviewers or students of the other teams with the same topic.
<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>
<ol>
<li>Pluralize the class. (StudentQuizzesController)</li>
<li>Rename methods to conform to RESTful style. (e.g. :- Rename the list method to index.)</li>
<li>Reduce the number of instance variables per controller action.</li>
<li>Review Method <b>graded?</b> for boolean zen.</li>
<li>Perform Code cleanup by removing unused 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>


 
==Modifications made to the Existing Code==
<b>Classes :</b> student_quiz_controller.rb<br>
<b>What it does : </b>This class records the quizzes that the student has attempted and its progress and also submits grades for the essays in the quizzes attempted by the student.<br>
<b>What has to be changed : </b>
*change list to index (and others in the RESTful style)
* Pluralize the class (StudentQuizzesController)
* graded? - review boolean zen
* Reduce the number of instance variables per controller action.
* Change the instance variables to local variables if they are not being used in the view.
* Modify methods to conform to RESTful style
* Code cleanup by using string interpolation instead of concatenation, replacing '==' with eql? and :key =>'value' with key: 'value', modifying declarations of Arrays and Hashes,removing commented out code
* Replace find_by with where to follow Rails 4.0 conventions
 
==Modification to Existing Code==


===Pluralized the class name StudentQuizController===
===Pluralized the class name StudentQuizController===
As per Rails convention the controller class names are sugested to be plural.
----
This helps in generating RESTful routing URI helpers. Also, naming the classes as plural seems more natural and helps in understanding<ref>http://stackoverflow.com/questions/646951/singular-or-plural-controller-and-helper-names-in-rails</ref>.  
As per the Rails convention the controller class names are suggested to be plural.
We used the refactor functionality in RubyMineto rename StudentQuiz controller class to StudentQuizzes.
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>.  
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.
The following files were modified and/or renamed.
<pre>
app/controllers/review_mapping_controller.rb
app/controllers/review_mapping_controller.rb
app/controllers/{student_quiz_controller.rb → student_quizzes_controller.rb}
app/controllers/{student_quiz_controller.rb → student_quizzes_controller.rb}
Line 49: Line 46:
test/unit/helpers/student_quiz_helper_test.rb
test/unit/helpers/student_quiz_helper_test.rb
test/unit/helpers/student_quizzes_helper_test.rb
test/unit/helpers/student_quizzes_helper_test.rb
</pre>


The detailed changeset for this refactor can be seen [https://github.com/ankit3005/expertiza/commit/20981333a14a3f468f76cd6d414b3088975b71bd here].  
The detailed change-set for this refactor can be seen [https://github.com/ankit3005/expertiza/commit/20981333a14a3f468f76cd6d414b3088975b71bd here].
 
 


===RESTful style implementation===
===RESTful style implementation===
* Modifications to student_quiz_controller.rb :
----
The purpose of the list method in the Student Quizzes controller is to list all the quizzes that are available to this 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 index. 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_quiz_controller (For e.g. :- The view finished_quiz.html.erb of the Student Quiz controller, the review_mapping controller.rb etc.).
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.).


Before Refactoring :
<b>Before Refactoring :</b>
</code>
<br> Method : list
<pre>
def list
def list
     @participant = AssignmentParticipant.find(params[:id])
     @participant = AssignmentParticipant.find(params[:id])
Line 94: Line 91:
     end
     end
   end
   end
</code>
}
</pre>


After Refactoring :
<b>After Refactoring :</b>
 
<br> Method : index
<code>
<pre>
def index
def index
     participant = AssignmentParticipant.find(params[:id])
     participant = AssignmentParticipant.find(params[:id])
Line 105: Line 103:
     @quiz_mappings = QuizResponseMap.get_mappings_for_reviewer(participant.id)
     @quiz_mappings = QuizResponseMap.get_mappings_for_reviewer(participant.id)
end
end
</code>
</pre>


===Instance Variable Reductions===
===Instance Variable Reductions===
In Rails, 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</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</ref> Thus, we need to reduce coupling to the maximum possible extent and in this case it can be achieved by reducing as many instance variables as possible. Therefore, we have refactored our code in order to eliminate unused and unneccessary instance variables.
----
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.
 
In the <b>take_quiz method</b>, the following instance variables were removed:
<br>
* Changed the <b>quizzes</b> instance variable to a local variable.
* 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>
<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>


student_quizzes_controller.rb
===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>


In the take_quiz method, the following instance variables were removed
<b> Before Refactoring </b>
Changed the quizzes instance variable to a local variable.
<br>
Eliminated instance variable assignment which was not used anywhere.
Method : graded?
Eliminated local variable teams which was not used any where
The unified diff of the changes made is shown below
<pre>
<pre>
  def self.take_quiz assignment_id , reviewer_id
def graded?(response, question)
-    @quizzes = Array.new
  if Score.where(question_id: question.id, response_idresponse.id).first
+    quizzes = Array.new
    return true
    reviewer = Participant.where(user_id: reviewer_id, parent_id: assignment_id).first
  else
-    @assignment = Assignment.find(assignment_id)
    return false
-    teams = TeamsUser.where(user_id: reviewer_id)
  end
    Team.where(parent_id: assignment_id).each do |quiz_creator|
end
      unless TeamsUser.find_by_team_id(quiz_creator.id).user_id == reviewer_id
</pre>
        Questionnaire.where(instructor_id: quiz_creator.id).each do |questionnaire|
 
-          if !@assignment.team_assignment?
<b> After Refactoring </b>
-            unless QuizResponseMap.where(reviewed_object_id: questionnaire.id, reviewer_idreviewer.id).first
<br> Method : graded?
-              @quizzes.push(questionnaire)
<pre>
-            end
def graded?(response, question)
-          else unless QuizResponseMap.where(reviewed_object_id: questionnaire.id, reviewer_id:  reviewer_id).first
  return (Score.where(question_id: question.id, response_idresponse.id).first)
-            @quizzes.push(questionnaire)
  end
+          unless QuizResponseMap.where(reviewed_object_id: questionnaire.id, reviewer_idreviewer.id).first
+            quizzes.push(questionnaire)
          end
        end
      end
    end
+    return quizzes
  end
- return @quizzes
-end
</pre>
</pre>


===Code Cleanup===
===Code Cleanup===
Code cleanup in the controller so that the code conforms closely to Rails conventions and enhanced readability of code.
----
* Replaced String concats with #{} in paginate_list
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].
Before Refactoring :


===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>
==== Used .eql? instead of "==" ====
----
<b>Before Refactoring</b>
<br> Method : finished_quiz
<pre> if score.score == -1 </pre>
<b>After Refactoring</b>
<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>
<pre>
if params["#{question.id}"] == nil
</pre>


<b>After Refactoring</b>
<br> Method: calculate_score
<pre>
if params["#{question.id}"].nil?
</pre>
</pre>


After Refactoring :
==== Used a Boolean variable when that is sufficient====
----
<b>Before Refactoring</b>
<br> Method: record_response
<pre>
valid = 1
if valid == 0
</pre>


<b>After Refactoring</b>
<br> Method: calculate_score
<pre>
<pre>
valid = false
if valid
</pre>
====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>


<b>After Refactoring</b>
<br> Method: calculate_score
<pre>
if new_score.comments.empty? || new_score.comments.nil?
  valid = false
</pre>
</pre>


* Replaced '==' with eql?
====Use of Routing Helpers====
Before Refactoring :
----
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>
<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>
</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>


After Refactoring :
<b>Before Refactoring</b>
<br> student_quizzes_controller.rb
<br> Method : index
<pre>
<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>
</pre>


* Replaced :key =>'value' with key: 'value'
===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:


Before Refactoring :
* 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>
<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>
</pre>


*Modified declarations of Arrays and Hashes
----
Before Refactoring :
* 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>
<pre>
#index
correct_answer = QuizQuestionChoice.where(question_id: question.id, iscorrect: 1)
@letters = Array.new
</pre>


#paginate_list
<b>After Refactoring</b>
paginate_options = {"1" => 25, "2" => 50, "3" => 100}
<pre>
correct_answers = QuizQuestionChoice.where(question_id: question.id, iscorrect: true)
</pre>
</pre>


After Refactoring :
----
* 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>
<pre>
<pre>
#index
# First Query
@letters =[]
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>


#paginate_list
<b>After Refactoring</b>
paginate_options = Hash["1" ,25 , "2", 50, "3", 100]
<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>
</pre>


*Removed unused methods like self.participants_in and commented out code.
----
* 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


===Use of Routing Helpers===
unless new_score.valid?
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.  
valid = 1
end
new_scores.push(new_score)


Before Refactoring:
# Part 2  
<br>
new_scores.each do |score_update|
score_update.score = score
scores.push(score_update)
end
</pre>
<b>After Refactoring</b>
<pre>
<pre>
# Part 1
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)
# Part 2
scores.each do |score_update|
  score_update.score = score
end
</pre>
</pre>
<br>
 
After Refactoring
----
<br>
* 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>
<pre>
def show_selection
# Part 1 of the Scoring Logic
      @user = User.where(name: params[:user][:name]).take
questions.each do |question|
      if @user != nil
score = 0
        role=get_role
if (QuestionType.find_by_question_id question.id).q_type == 'MCC'
        if role.parent_id.eql? nil || role.parent_id < (session[:user]).role_id || @user.id.eql?(session[:user]).id
  score = 0
          render action: 'show'
  if params["#{question.id}"] == nil
        else
  valid = 1
          flash[:note] = 'The specified user is not available for editing.'
  else
          redirect_to users_path  #Replaced URL with routing helper
    correct_answer = QuizQuestionChoice.where(question_id: question.id, iscorrect: 1)
        end
    params["#{question.id}"].each do |choice|
      else
    correct_answer.each do |correct|
        flash[:note] = params[:user][:name]+' does not exist.'
    if choice == correct.txt
      redirect_to users_path  #Replaced URL with routing helper
       score += 1
       end
     end
     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>
</pre>
<br>
<b>After Refactoring</b>
<pre>
# Part 1 of the Scoring Logic
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


===Replace find_by_.. with where in querying===
# Part 2 of the scoring logic - We have also compared the number of options the user selected to the total number of correct answers
Rails 4 conventions dictate the use of 'where()' over the use of 'find_by_...' methods while querying ActiveRecords. The code has been refactored to replace the usages of find_by.. with where().
if score.eql? correct_answers.count && score == params["#{question.id}"].count
  score = 1
else
  score = 0
end
</pre>


Before Refactoring
----
* 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>
<pre>
<pre>
def record_response
</pre>
  @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)
  scores = Array.new
  new_scores = Array.new
  valid = 0
  questions = Question.where(questionnaire_id: @questionnaire.id)
  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|


After Refactoring
          correct_answer.each do |correct|
<pre>
            if choice == correct.txt
              score += 1
</pre>
            end


===Replace controller method with a model method ===
          end
Rails 4 conventions dictate the use of 'where()' over the use of 'find_by_...' methods while querying ActiveRecords. The code has been refactored to replace the usages of find_by.. with where().
          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


Before Refactoring
end
</pre>
 
<b>After Refactoring</b>
<pre>
<pre>
     @quiz_mappings = QuizResponseMap.where(reviewer_id: participant.id)
# New record_response method
</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


    calculate_score map,response


After Refactoring
  end
<pre>
@quiz_mappings = QuizResponseMap.get_mappings_for_reviewer(participant.id)
</pre>


# 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>


==References==
==References==
<references/><br>
<references/><br>

Latest revision as of 02:46, 6 November 2014

Expertiza - Refactoring StudentQuizController

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 StudentQuiz controller. In this Wiki Page, we would be explaining the changes that we have made for the same.

Project Description

The StudentQuiz controller consists of methods involved in creating, scoring & recording responses of the quizzes taken by reviewers or students of the other teams with the same topic.
The changes that are needed to be done are described as follows:<ref>GoogleDoc for our project requirements</ref>

  1. Pluralize the class. (StudentQuizzesController)
  2. Rename methods to conform to RESTful style. (e.g. :- Rename the list method to index.)
  3. Reduce the number of instance variables per controller action.
  4. Review Method graded? for boolean zen.
  5. Perform Code cleanup by removing unused code.
  6. Use good Ruby style guidelines in the code.
  7. Split a method performing multiple tasks, into seperate methods.
  8. Fix logical errors in the code.

Modifications made to the Existing Code

Pluralized the class name StudentQuizController


As per the Rails convention the controller class names are suggested to be plural. This helps in generating RESTful routing URI helpers. Also, naming the classes as plural seems intuitive.<ref>Using the Singular or Plural controller and helper names in Rails</ref>. We used the refactor functionality in RubyMine to rename the StudentQuiz controller class to StudentQuizzes.

The following files were modified and/or renamed.

app/controllers/review_mapping_controller.rb
app/controllers/{student_quiz_controller.rb → student_quizzes_controller.rb}
app/helpers/student_quiz_helper.rb
app/helpers/student_quizzes_helper.rb
app/views/questionnaires/view.html.erb
app/views/{student_quiz → student_quizzes}/_quiz_form.html.erb
app/views/{student_quiz → student_quizzes}/_responses.html.erb
app/views/{student_quiz → student_quizzes}/_set_dynamic_quiz.html.erb
app/views/{student_quiz → student_quizzes}/_set_self_quiz.html.erb
app/views/{student_quiz → student_quizzes}/finished_quiz.html.erb
app/views/{student_quiz → student_quizzes}/grade_essays.html.erb
app/views/{student_quiz → student_quizzes}/list.html.erb
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

The detailed change-set for this refactor can be seen here.

RESTful style implementation


The purpose of the list method in the StudentQuizzes controller 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 index. 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.).

Before Refactoring :
Method : list

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
}

After Refactoring :
Method : index

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

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>Decreasing Controller-View Coupling in Rails</ref>. Increased coupling results in several problems, including less maintainability of code, difficulty in code reuse etc. <ref>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.

In the take_quiz method, the following instance variables were removed:

  • Changed the quizzes instance variable to a local variable.
  • Eliminated the instance variable assignment which was not being used anywhere.
  • Eliminated the local variable teams which was not being used any where

Before Refactoring

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

After Refactoring

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

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 graded? that does this thing. Thus, we eliminated the if..else construct in the graded? method as it was redundant.<ref>About Boolean Zen</ref>

Before Refactoring
Method : graded?

 def graded?(response, question)
  if Score.where(question_id: question.id, response_id:  response.id).first
    return true
  else
    return false
  end
 end

After Refactoring
Method : graded?

 def graded?(response, question)
  return (Score.where(question_id: question.id, response_id:  response.id).first)
 end

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 @quiz_phase and the @num_quizzes_total variables because they were not being used anywhere in the code. Also, since the local variables quiz_due_date, deadline_type_id, participant were not being used anywhere, we could eliminate the entire if block from the code below. We then converted the instance variable participant 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 RESTful style implementation.

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:-

Used .eql? instead of "=="


Before Refactoring
Method : finished_quiz

 if score.score == -1 

After Refactoring

 if score.score.eql? -1 

Eliminated the "== true/false" check


Before Refactoring
Method : finished_quiz

 if essay_not_graded == true 

After Refactoring

 if essay_not_graded 

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


Before Refactoring
Method: record_response

 elsif  correct_answer and params["#{question.id}"] == correct_answer.txt 

After Refactoring
Method: calculate_score

 elsif  correct_answer && params["#{question.id}"]== correct_answer.txt 

Use key: ‘value’, not :key => ‘value’


Before Refactoring
Method: record_response

new_score = Score.new :comments => choice, :question_id => question.id, :response_id => @response.id

After Refactoring
Method: calculate_score

new_score = Score.new comments: choice, question_id: question.id, response_id: response.id

Replace find_by_... with a where command


Before Refactoring
Method: record_response

 if (QuestionType.find_by_question_id question.id).q_type == 'MCC' 

After Refactoring
Method: calculate_score

ques_type = (QuestionType.where( question_id: question.id)).q_type
if ques_type.eql? 'MCC' 

Used .nil? instead of "== nil"


Before Refactoring
Method: record_response

if params["#{question.id}"] == nil

After Refactoring
Method: calculate_score

if params["#{question.id}"].nil?

Used a Boolean variable when that is sufficient


Before Refactoring
Method: record_response

valid = 1
 if valid == 0

After Refactoring
Method: calculate_score

valid = false
 if valid

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.

Before Refactoring
Method: record_response

unless new_score.comments != "" && new_score.comments
   valid = false

After Refactoring
Method: calculate_score

if new_score.comments.empty? || new_score.comments.nil?
   valid = false

Use of Routing Helpers


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.

Before Refactoring:
config.rb does not contain a student_quizzes resource
review_mapping_controller.rb

redirect_to :controller => 'student_quizzes', :action => 'index', :id => params[:participant_id]

After Refactoring
config.rb

resources :student_quizzes, :only => [:index]

review_mapping_controller.rb

redirect_to student_quizzes_path(:id => params[:participant_id])

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 quiz_response_map model, since it queries that particular table in the database. The code was extracted into the get_mappings_for_reviewer method and placed in the file quiz_response_map.rb. This method was then called from the StudentQuizzes controller. <ref name = "stackoverflow">Fat Models and Skinny Controllers.</ref>

Before Refactoring
student_quizzes_controller.rb
Method : index

 @quiz_mappings = QuizResponseMap.where(reviewer_id: participant.id)


After Refactoring
student_quizzes_controller.rb
Method : index

 @quiz_mappings = QuizResponseMap.get_mappings_for_reviewer(participant.id)


quiz_response_map.rb

def self.get_mappings_for_reviewer(participant_id)
  return QuizResponseMap.where(reviewer_id: participant_id)
end

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

Before Refactoring

questions.each do |question|		
  score = 0		
  if (QuestionType.find_by_question_id question.id).q_type == 'MCC'		
    score = 0

After Refactoring

questions.each do |question|
 score = 0
 if ques_type.eql? 'MCC'
    # Eliminated score = 0 over here

  • The variable correct_answer stored multiple values as the where condition to which it was assigned was returning multiple values. Therefore it seemed more intuitive to rename correct_answer to correct_answers so that it is apparent that it contains multiple values.

Before Refactoring

correct_answer = QuizQuestionChoice.where(question_id: question.id, iscorrect: 1)

After Refactoring

correct_answers = QuizQuestionChoice.where(question_id: question.id, iscorrect: true)

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

Before Refactoring

# 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'

After Refactoring

# 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'

  • The new_scores and scores array stored almost the similar values. The scores array contained a copy of the value that the new_scores array contained. Therefore we eliminated the new_scores array and are performing all the operations only on the scores array.

Before Refactoring

# 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

After Refactoring

# Part 1
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)
	
# Part 2		
scores.each do |score_update|
  score_update.score = score
end

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

Before Refactoring

# 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

After Refactoring

# Part 1 of the Scoring Logic
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

# 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

  • The record_response 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 calculate_score that would calculate the score for the questions and record_response now only performs the task of saving responses to the database. By doing this, we followed the Single Responsibility O-O Design Principle for methods mentioned by Mr. Robert C. Martin which states that "Every software module should have only one reason to change"<ref>O-O Design Principles</ref>

Before Refactoring

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)
  scores = Array.new
  new_scores = Array.new
  valid = 0
  questions = Question.where(questionnaire_id: @questionnaire.id)
  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

          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

After Refactoring

# New record_response method
  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

    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

References

<references/>