CSC/ECE 517 Spring 2021 - E2102. Refactor quiz questionnaires controller.rb

From Expertiza_Wiki
Jump to navigation Jump to search

Members

  • Nicholas Himes
  • Surya Makthal

Introduction

In order for students to write effective reviews, they need to understand the work that they are reviewing. Expertiza can allow authors to create a quiz over their work. If an assignment is set up to use quizzes, then a reviewer must take the quiz over the author’s work before reviewing it. Then, based on how the reviewer scores on that quiz, Expertiza can weight that author’s scores accordingly in calculating a peer grade. Thus, reviewers who understand the author’s work better have more say in determining the author’s (peer) grade.

Problem Statement

quiz_questionnaires_controller, naturally, is the controller that administers a quiz. A quiz_questionnaire is a kind (subclass) of questionnaire in Expertiza. A questionnaire consists of a number of questions (or “items”) that the reviewer needs to answer. This questionnaire contains many violations of good programming practices. E2102 deals with refactoring the code inside quiz_questionnaires_controller.rb so that it is clear and conforms to DRY principles.

Issues

Issues and their progress/comments can be viewed in much more detail on our repository project board [1].

  • action_allowed? needs to be refactored to use authorization utilities (see project E1915 on the Expertiza wiki)
  • Lines 23-24 contain useless comments; remove.
  • Line 26 needs a comment saying what a valid request is.
  • Line 26: if !assignment.require_quiz? ⇒ unless assignment.require_quiz?
  • Line 31: Change message to, “This assignment is not configured to use quizzes.”
  • Line 34: team_check is an unacceptable method name; the method name needs to say what the method does. Also the comment at the end of the line is useless; replace it by one that says what is going on.
  • Line 50: Why isn’t if valid.eql?("valid") simply if valid?

There also needs to be a comment saying what “valid” means in this context.

  • Comment at line 52 is confusing.
  • Line 55 needs a comment saying what it is doing.
  • Line 78: Your quiz has been taken by some other students, you cannot edit it anymore ⇒ Your quiz has been taken by other students; you cannot edit it anymore
  • create and update need to share code. Looks like create is missing some functionality present in update.
  • Line 100: What about other question types, e.g., Criterion? Or are these not permitted in quizzes? If so, what stops them from being used?

I’m not clear on what a “choice” is; please explain.

  • Line 133: Seems like this should be a method of assignment_participant; then the participant_id can be the receiver.

Suggested name: has_team_and_topic

  • Line 146: Need an explanation (comment) about what a valid question is.
  • Lines 165 ff.: Why are the update methods before the create methods?

Comment on Line 159 should say, # update checkbox question But, “question” should be replaced with “item” throughout. Why is code so different between update & create methods? Should this code be moved to the model files for the appropriate question types?

  • Line 244?: I think it would be good to flash a message before deleting a question.
  • Line 255: Not clear what a “choice type” is.
  • Lines 264 to 272: Big if-statement checks types & creates correct type of question. This should be changed to use the Factory Method pattern, shared by code for creating rubric items and code for creating quiz questions.

Code Improvements

Note: The majority of the issues listed in the previous section were solved simply by adding descriptive comments/messages or improving existing ones to clarify behavior, so those will not be shown in this section.

action_allowed? needs to be refactored to use authorization utilities

Prior to changes, the code in the action_allowed? method had some repeated code. 

#Quiz questionnaire edit option to be allowed for student
def action_allowed?
  if params[:action] == "edit"
    @questionnaire = Questionnaire.find(params[:id])
    (['Super-Administrator', 'Administrator'].include? current_role_name) ||
        (['Student'].include? current_role_name)
  else
    ['Super-Administrator',
     'Administrator',
     'Instructor',
     'Teaching Assistant', 'Student'].include? current_role_name
  end
end

This can be solved by using the authorization helper methods described in E1915 Authorization Utilities. 

include AuthorizationHelper

#Quiz questionnaire edit option to be allowed for student
def action_allowed?
  if params[:action] == "edit"
    @questionnaire = Questionnaire.find(params[:id])
    current_user_has_admin_privileges? || current_user_is_a?("Student")
  else
    current_user_has_student_privileges?
  end
end

Line 26: if !assignment.require_quiz? ⇒ unless assignment.require_quiz?

Some logic inside the new function of the controller is technically correct... 

if !assignment.require_quiz? # flash error if this assignment does not require quiz
  flash[:error] = "This assignment does not support the quizzing feature."

...but can be improved by making better use of Ruby keywords. 

unless assignment.require_quiz? # flash error if this assignment does not require quiz
  flash[:error] = "This assignment is not configured to use quizzes."

Line 34: team_check is an unacceptable method name; the method name needs to say what the method does. Also the comment at the end of the line is useless; replace it by one that says what is going on.

team_check is not a descriptive method name. While not explicitly stated in the issue statement, the logic within the method unnecessarily assigns boolean values to a local variable. 

def team_check(participant_id, assignment)
  valid_request = true
  team = AssignmentParticipant.find(participant_id).team
  if team.nil? # flash error if this current participant does not have a team
    flash[:error] = "You should create or join a team first."
    valid_request = false
  elsif assignment.topics? && team.topic.nil? # flash error if this assignment has topic but current team does not have a topic
    flash[:error] = "Your team should have a topic."
    valid_request = false
  end
  valid_request # return whether the request is valid or not
end

Thus, we rename the method to team_valid? and simplify the logic. 

def team_valid?(participant_id, assignment)
  team = AssignmentParticipant.find(participant_id).team
  if team.nil? # flash error if this current participant does not have a team
    flash[:error] = "You should create or join a team first."
    false
  elsif assignment.topics? && team.topic.nil? # flash error if this assignment has topic but current team does not have a topic
    flash[:error] = "Your team should have a topic."
    false
  else # the current participant is part of a team that has a topic
    true
  end
end

Lines 264 to 272: Big if-statement checks types & creates correct type of question. This should be changed to use the Factory Method pattern, shared by code for creating rubric items and code for creating quiz questions.

Within the save_choices function of the controller, there is an if-statement that determines what function to use when saving a question of a certain type, which can be either true/false or multiple choice. 

q_answer_choices.each_key do |choice_key|
  if q_type == "TrueFalse"
    create_truefalse(question, choice_key, q_answer_choices)
  else # create MultipleChoice of either type, rather than creating them separately based on q_type
    create_multchoice(question, choice_key, q_answer_choices)
  end
end

We can make this more readable by substituting this structure with a function call to a newly defined factory method called question_factory... 

question_factory(q_type, question, choice_key, q_answer_choices) # allow factory method to create appropriate question

...which contains the refactored if-else structure and saves questions of the appropriate type. 

# factory method to create the appropriate question based on the question type (true/false or multiple choice)
def question_factory(q_type, question, choice_key, q_answer_choices)
  if q_type == "TrueFalse"
    create_truefalse(question, choice_key, q_answer_choices)
  else # create MultipleChoice of either type, rather than creating them separately based on q_type
    create_multchoice(question, choice_key, q_answer_choices)
  end
end

Code improvements can be viewed in much more detail in the E2102 pull request [2].

Coverage Note

Coverage testing was disabled on our PR like many others so the most recent coverage is not available. However, the only change we had to make to a rspec file in our project was for the issue of *Line 31: Change message to, “This assignment is not configured to use quizzes.”. You can see in our file changes we simply fixed the expectations of the test case and the build passed for our controller file.

Test Plan

Note: Our project is a refactoring of existing controller code. It cleaned up the controller and therefore does not really have anything to test besides if the app is still working.

Steps:

1) Visit our hosted website (until the end of April 2021) at http://152.7.99.72:8080/

2) Login with the username instructor6 and password password

3) Click assignments at the top (to the right of "Courses" under "Manage Content")

4) For "773HimesMakthalTest" click the edit pencil icon

5) Note that Has Quiz? is enabled and it has due dates and is assigned to student7366

6) At the top, go to Manage > Impersonate User > student7366

7) Under assignments, click the 773HimesMakthalTest assignment

8) Click "Your work" > Click "Create a quiz" if no quiz has been made yet

9) You can fill in the new quiz or edit an existing one. Also you can view the quiz.

Again, this all worked before we made any changes. Our project was just refactoring.

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2021_-_E2102._Refactor_quiz_questionnaires_controller.rb&oldid=138361"