CSC/ECE 517 Fall 2014/oss E1460 aua

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

Our Goal in this project is to refactor the StudentQuiz controller. 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. The changes that are needed to be done are described as follows:<ref>GoogleDoc for our project requirements</ref>

Pluralize the class. (StudentQuizzesController)
Rename methods to conform to RESTful style. (e.g. :- Rename the list method to index.)
Reduce the number of instance variables per controller action.
Review Method graded? for boolean zen.
Perform Code cleanup by removing unused code.
Use good Ruby style guidelines in the code.
Split a method performing multiple tasks, into seperate methods.
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

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

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

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