CSC/ECE 517 Fall 2014/oss E1460 aua

Expertiza - Refactoring StudentQuizController

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 here. The source code can be forked and cloned for making modifications. ertiza with main focus of Refactoring the Users Controller.

Project Description

Classes : student_quiz_controller.rb
What it does : 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.
What has to be changed :

• 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

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>. We used the refactor functionality in RubyMineto rename 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 changeset for this refactor can be seen here.

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

Before Refactoring :

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 :

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

Before Refactoring

student_quizzes_controller.rb

In the take_quiz method, the following instance variables were removed Changed the quizzes instance variable to a local variable. Eliminated instance variable assignment which was not used anywhere. Eliminated local variable teams which was not used any where The unified diff of the changes made is shown below

  def self.take_quiz assignment_id , reviewer_id
-    @quizzes = Array.new
+    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)
+          unless QuizResponseMap.where(reviewed_object_id: questionnaire.id, reviewer_id:  reviewer.id).first
+            quizzes.push(questionnaire)
           end
         end
       end
     end
+    return quizzes
   end
-  return @quizzes
-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.
Before Refactoring

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

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

• Replaced :key =>'value' with key: 'value'

Before Refactoring :

• Modified declarations of Arrays and Hashes

Before Refactoring :

After Refactoring :

• Removed unused methods like self.participants_in and commented out code.

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:

After Refactoring

Replace find_by_.. with where in querying

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().

Before Refactoring

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

Used .eql? instead of "=="

Before Refactoring

 if score.score == -1 

After Refactoring

 if score.score.eql? -1 

Eliminated the "== true/false" check

Before Refactoring

 if essay_not_graded == true 

After Refactoring

 if essay_not_graded 

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

Before Refactoring

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

After Refactoring

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

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

Before Refactoring

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

Before Refactoring

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

Replace find_by_... with a where command

Before Refactoring

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

After Refactoring

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

Use good array checking

[].empty? # not [].length == 0 or [].length.zero? [:foo].any? # not [:foo].length > 0 [:foo].one? # not [:foo].length == 1 [:foo].first # not [:foo][0] [:foo].last # not [:foo][-1]

Use find_each for efficient loops over models

SignedUpUser.all.each do |user| # BAD SignedUpUser.find_each do |user| # GOOD

Use good conditional statements

The boolean itself would suffice as an if condition.
It is a good practice to use a boolean variable if we only need a boolean for our purpose.

Before Refactoring

valid = 1
 if valid == 0

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

After Refactoring

valid = false
 if valid

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

Replace controller method with a model method

Rails 4 conventions dictate the use of a fat model and skinny controller. It is better to put 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 DB. The code was extracted into the subsequent method displayed below and called from the invitation controller.<ref name = "stackoverflow">stackoverflow</ref>

Before Refactoring

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

After Refactoring

 @quiz_mappings = QuizResponseMap.get_mappings_for_reviewer(participant.id)

References

<references/>