CSC/ECE 517 Fall 2020 - E2070. Refactor response controller.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
Line 38: Line 38:


== Refactor & Code Modification ==
== Refactor & Code Modification ==
We did the following refactor to make this method more clear:


=== METHOD: def assign_instance_vars ===
=== METHOD: def assign_instance_vars ===
We did the following refactor to make this method more clear:
 
*After inspection of the code, the instance variable is actually action handles to the views from controller including "new" and "edit", thus refactor method name to assign_action_parameter would be better to understand.
*the instance variables are actually action handles to the views from controller including "new" and "edit", thus refactoring method name to assign_action_parameters would be better to understand what this method is trying to do.
*This method is a private helper method and is used in new and edit methods to set the action parameters for the new and edit actions
 
[[File:E2070 assign instance.png|1200px]]
[[File:E2070 assign instance.png|1200px]]
*After renaming, the method became more clear of what it's doing
[[File:E2070 action parameters.png]]


=== METHOD: def scores ===
=== METHOD: def scores ===
*After inspection of the code, this method was already defined in other class and is useless in the current class, thus remove it would be a reasonable choice.
 
*This method was already defined in other classes and is useless in the current class, thus remove it would be a reasonable choice.
 
[[File:E2070 Scores.png|1200px]]
[[File:E2070 Scores.png|1200px]]


===METHOD: def set_questionnaire===
===METHOD: def set_questionnaire===
In the method set_content in response_controller.rb, there is one line of code that "new_response ? set_questionnaire_for_new_response : set_questionnaire". Thus, we can tell the method set_qustionnaire will called when new_response is false, which means that the object was triggered by an existing response. In the method set_questionnaire, the line of code that “@questionnaire = @response.questionnaire_by_answer(answer)”, which is looking up the questionnaire answered by this existing response. Therefore, refactoring the method name to find_quesetionnaire would be better to understand.  
 
*In the method set_content in response_controller.rb, there is one line of code that "new_response ? set_questionnaire_for_new_response : set_questionnaire". Thus, we can tell the method set_qustionnaire will called when new_response is false, which means that the object was triggered by an existing response. In the method set_questionnaire, the line of code that “@questionnaire = @response.questionnaire_by_answer(answer)”, which is looking up the questionnaire answered by this existing response. Therefore, refactoring the method name to find_quesetionnaire would be better to understand.  
[[File:E2070 set questionnaire.png|1200px]]
[[File:E2070 set questionnaire.png|1200px]]


===METHOD: def set_questionnaire_for_new_response===
===METHOD: def set_questionnaire_for_new_response===
In the method set_content in response_controller.rb, there is one line of code that "new_response ? set_questionnaire_for_new_response : set_questionnaire". Thus, we can tell the method set_qustionnaire will called when new_response is true, which means that the object was triggered by a new response. In the set_questionnaire_for_new_response method, the "cast" statement informs us that the new questionnaire are created based on the types or Reviewers, such as Review, Metareview, Teammate Review and so on. Therefore, refactoring the set_questionnaire_for_new_response method to create_new_questionnaire would be better to understand.  
 
*In the method set_content in response_controller.rb, there is one line of code that "new_response ? set_questionnaire_for_new_response : set_questionnaire". Thus, we can tell the method set_qustionnaire will called when new_response is true, which means that the object was triggered by a new response. In the set_questionnaire_for_new_response method, the "cast" statement informs us that the new questionnaire are created based on the types or Reviewers, such as Review, Metareview, Teammate Review and so on. Therefore, refactoring the set_questionnaire_for_new_response method to create_new_questionnaire would be better to understand.  
[[File:E2070 set questionnaire for new.png|1200px]]
[[File:E2070 set questionnaire for new.png|1200px]]


===METHOD: def show_calibration_results_for_student ===
===METHOD: def show_calibration_results_for_student ===
[[File:E2070 calibration controller.png|1200px]]
[[File:E2070 calibration controller.png|1200px]]
[[File:E2070 calibration model.png|1200px]]
[[File:E2070 calibration model.png|1200px]]

Revision as of 16:08, 14 October 2020

Project Background

A response is the object that is created when someone fills out a review rubric, such as when one writes a review, gives feedback to a reviewer, or fills out a survey. Responses to the individual rubric items are kept in Answer objects; each Answer object has a response_id to say what Response it is part of. Since response_controller needs to work with many kinds of responses, its code is pretty general. It is not the worst controller in the system, but it would be much clearer if its method names were more descriptive of what they do.

Project Team Member

  • Chenwei Zhou (czhou6)
  • Tianrui Wang (twang33)
  • Xinran Li (xli56)

Goal of this Project

This project tries to modify and refactor the response_controller.rb in a more clear way

Files Involved

response_controller.rb

response.rb

Issue

Methods need to be fixed

  • def assign_instance_vars
  • def scores
  • def new
  • def set_questionnaire
  • def set_questionnaire_for_new_response
  • def show_calibration_results_for_students

Recommandation from mentor

Project Lists

  • def assign_instance_vars : This name could be more specific.
  • def scores : This should be a model method (in response.rb)! It is all calculation.
  • def new : This method contains a complicated condition that determines whether the submission has been updated since the last time it was reviewed. If it has not, then the reviewer can edit his/her previous review. If there has been an update, then the reviewer gets a new review form to “update” the review. It would make sense to have a model method that tests whether there has been a review since the last file or link was submitted. Then the code here would just call that function. It would be a lot clearer what the new method is doing. set_content(new_response = false) also plays a role in this calculation. Perhaps this method should also be included in the refactoring, for the sake of clarity of the resulting code.
  • def set_questionnaire : Badly named; what kind of questionnaire and why? The name should be a lot clearer.
  • def set_questionnaire_for_new_response : Badly named, no comments, not at all clear. The logic is not like set_questionnaire. Rename, refactor for clarity, and add comments as appropriate.
  • def show_calibration_results_for_student : This method makes about five db accesses. Can it be broken into 2 methods, with the business logic moved to response.rb?

Refactor & Code Modification

We did the following refactor to make this method more clear:

METHOD: def assign_instance_vars

  • the instance variables are actually action handles to the views from controller including "new" and "edit", thus refactoring method name to assign_action_parameters would be better to understand what this method is trying to do.
  • This method is a private helper method and is used in new and edit methods to set the action parameters for the new and edit actions

  • After renaming, the method became more clear of what it's doing

METHOD: def scores

  • This method was already defined in other classes and is useless in the current class, thus remove it would be a reasonable choice.

METHOD: def set_questionnaire

  • In the method set_content in response_controller.rb, there is one line of code that "new_response ? set_questionnaire_for_new_response : set_questionnaire". Thus, we can tell the method set_qustionnaire will called when new_response is false, which means that the object was triggered by an existing response. In the method set_questionnaire, the line of code that “@questionnaire = @response.questionnaire_by_answer(answer)”, which is looking up the questionnaire answered by this existing response. Therefore, refactoring the method name to find_quesetionnaire would be better to understand.

METHOD: def set_questionnaire_for_new_response

  • In the method set_content in response_controller.rb, there is one line of code that "new_response ? set_questionnaire_for_new_response : set_questionnaire". Thus, we can tell the method set_qustionnaire will called when new_response is true, which means that the object was triggered by a new response. In the set_questionnaire_for_new_response method, the "cast" statement informs us that the new questionnaire are created based on the types or Reviewers, such as Review, Metareview, Teammate Review and so on. Therefore, refactoring the set_questionnaire_for_new_response method to create_new_questionnaire would be better to understand.

METHOD: def show_calibration_results_for_student

show_calibration page

Teacher's action to triggle this function

After calibration function is activated

METHOD: def new

Create new response

After NEW action

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2020_-_E2070._Refactor_response_controller.rb&oldid=135204"