CSC/ECE 517 Spring 2022 - E2217: Refactor questionnaires controller: Difference between revisions
No edit summary |
|||
(19 intermediate revisions by 3 users not shown) | |||
Line 2: | Line 2: | ||
== Description == | == Description == | ||
This project focused on | This project focused on refactoring <code>questionnaires_controller.rb</code> by sanding down rough edges of code throughthe removal of the unused ''create_questionnaire'' method, breaking up the exceedingly long ''create'' method, transfer of hardcoded values in the ''add_new_questions'' method, and general code cleanup. | ||
== Problems and Solutions == | == Problems and Solutions == | ||
===='''Problem''': The <code>create_questionnaire</code> method is not used | ===='''Problem''': The <code>create_questionnaire</code> method is not used==== | ||
*'''Solution''': | <code>create_questionnaire</code> has no immediate apparent calls, and appears to have the same functionality as <code>create</code> | ||
*'''Solution''': <code>create_questionnaire</code> method and its corresponding test has been removed because it was not used anywhere. | |||
==== '''Problem''': The <code>create</code> method is 49 lines long | ==== '''Problem''': The <code>create</code> method is 49 lines long==== | ||
*'''Solution''': | *'''Solution''':<code>@questionnaire</code> initialization was moved to a new method called <code>initialize_values</code>. The questionnaire node initialization was moved to <code>create_questionnaire_node</code>. | ||
[[File:Initialize values.JPG]] | |||
===='''Problem''': There are assignment values hardwired into the code in <code>add_new_questions</code> | |||
[[File:Create questionnaire node.JPG]] | |||
===='''Problem''': There are checks for whether a question is a <code>QuizQuestionnaire</code>==== | |||
These checks are likely unnecessary, and testing the class of an object carries potential problems. | |||
*'''Solution''': The first check was located in the <code>create_questionnaire</code> and has been deleted along with the method. | |||
===='''Problem''': There are assignment values hardwired into the code in <code>add_new_questions</code>==== | |||
Although these fields are okay to default to the given values, these should be defined constants | |||
*'''Solution''': As these values related to <code>ScoredQuestion</code> items, constants were created in <code>scored_question.rb</code>. <code>add_new_questions</code> now uses these class constants when assigning default values for fields. These values are all changeable in the UI after initialization. | *'''Solution''': As these values related to <code>ScoredQuestion</code> items, constants were created in <code>scored_question.rb</code>. <code>add_new_questions</code> now uses these class constants when assigning default values for fields. These values are all changeable in the UI after initialization. | ||
Line 21: | Line 29: | ||
[[File:Scored_question_changes.png]] | [[File:Scored_question_changes.png]] | ||
===='''Problem''': Several methods have a <code>questionnaire_id</code> as a parameter, this may be unnecessary in several situations where <code>params[:id]</code> would suffice | ===='''Problem''': Several methods have a <code>questionnaire_id</code> as a parameter==== | ||
Although not functionally broken, this may be unnecessary in several situations where <code>params[:id]</code> would suffice | |||
*'''Solution''': Where applicable, <code>questionnaire_id</code> was changed to <code>params[:id]</code>. This was done in methods where <code>params[:id].nil?</code> was not called, and in methods where <code>questionnaire_id = params[:id]</code> was called | *'''Solution''': Where applicable, <code>questionnaire_id</code> was changed to <code>params[:id]</code>. This was done in methods where <code>params[:id].nil?</code> was not called, and in methods where <code>questionnaire_id = params[:id]</code> was called | ||
Line 30: | Line 39: | ||
===='''Problem''': Several bad <var>if</var> statements exist==== | ===='''Problem''': Several bad <var>if</var> statements exist==== | ||
*'''Solution''': Line 96: <code>if @questionnaire.type != "QuizQuestionnaire" </code> checking if it is not a QuizQuestionnaire is not necessary so this line has been removed because the QuizQuestionnaire is same as the other questionnaire types. | *'''Solution''': Line 96: <code>if @questionnaire.type != "QuizQuestionnaire" </code> checking if it is not a QuizQuestionnaire is not necessary so this line has been removed because the QuizQuestionnaire is same as the other questionnaire types. | ||
[[File:Line_91_check.JPG]] | [[File:Line_91_check.JPG]] | ||
Line 190 : <code>if question.is_a? ScoredQuestion</code> check for ScoredQuestion has been removed.Also hardcoded the minlabel and max label values from <code>question.max_label = 'Strongly agree'</code> <code>question.min_label = 'Strongly disagree'</code> to | Line 190 : <code>if question.is_a? ScoredQuestion</code> check for ScoredQuestion has been removed.Also hardcoded the minlabel and max label values from <code>question.max_label = 'Strongly agree'</code> <code>question.min_label = 'Strongly disagree'</code> to | ||
Line 42: | Line 48: | ||
== Modified Files == | == Modified Files == | ||
<code>questionnaires_controller.rb</code> | |||
<code>scored_question.rb</code> | |||
<code>questionnaires_controller_spec.rb</code> | |||
== Testing == | == Testing == | ||
Line 51: | Line 62: | ||
==Future Improvements== | ==Future Improvements== | ||
*Check for <code>if question.is_a? ScoredQuestion</code> has been removed. | |||
*Explore if <code>create_questionnaire_node</code> can be moved to the <code>node</code> class | |||
*Explore moving <code>ScoredQuestion</code> initialization into the <code>scored_question.rb</code> model. | |||
==GitHub links and Pull Request== | |||
Link to Expertiza repository: [https://github.com/expertiza/expertiza here] | |||
Link to the forked repository: [https://github.com/War-Keeper/expertiza here] | |||
Link to Pull Request: [https://github.com/expertiza/expertiza/pull/2320 here] | |||
Latest revision as of 23:23, 21 March 2022
This page describes the changes made for the Spring 2022 OSS Project E2217: Refactoring Questionnaires Controller
Description
This project focused on refactoring questionnaires_controller.rb
by sanding down rough edges of code throughthe removal of the unused create_questionnaire method, breaking up the exceedingly long create method, transfer of hardcoded values in the add_new_questions method, and general code cleanup.
Problems and Solutions
Problem: The create_questionnaire
method is not used
create_questionnaire
has no immediate apparent calls, and appears to have the same functionality as create
- Solution:
create_questionnaire
method and its corresponding test has been removed because it was not used anywhere.
Problem: The create
method is 49 lines long
- Solution:
@questionnaire
initialization was moved to a new method calledinitialize_values
. The questionnaire node initialization was moved tocreate_questionnaire_node
.
Problem: There are checks for whether a question is a QuizQuestionnaire
These checks are likely unnecessary, and testing the class of an object carries potential problems.
- Solution: The first check was located in the
create_questionnaire
and has been deleted along with the method.
Problem: There are assignment values hardwired into the code in add_new_questions
Although these fields are okay to default to the given values, these should be defined constants
- Solution: As these values related to
ScoredQuestion
items, constants were created inscored_question.rb
.add_new_questions
now uses these class constants when assigning default values for fields. These values are all changeable in the UI after initialization.
Addition of constants to scored_question.rb
:
Problem: Several methods have a questionnaire_id
as a parameter
Although not functionally broken, this may be unnecessary in several situations where params[:id]
would suffice
- Solution: Where applicable,
questionnaire_id
was changed toparams[:id]
. This was done in methods whereparams[:id].nil?
was not called, and in methods wherequestionnaire_id = params[:id]
was called
Change of questionnaire_id
to params[:id]
in save_all_questions
:
Problem: Several bad if statements exist
- Solution: Line 96:
if @questionnaire.type != "QuizQuestionnaire"
checking if it is not a QuizQuestionnaire is not necessary so this line has been removed because the QuizQuestionnaire is same as the other questionnaire types.
Line 190 : if question.is_a? ScoredQuestion
check for ScoredQuestion has been removed.Also hardcoded the minlabel and max label values from question.max_label = 'Strongly agree'
question.min_label = 'Strongly disagree'
to
question.max_label = ScoredQuestion::DEFAULT_MAX_AGREEMENT
question.min_label = ScoredQuestion::DEFAULT_MIN_AGREEMENT
Modified Files
questionnaires_controller.rb
scored_question.rb
questionnaires_controller_spec.rb
Testing
Running Tests
rspec ./spec/controllers/questionnaires_controller_spec.rb
Future Improvements
- Check for
if question.is_a? ScoredQuestion
has been removed.
- Explore if
create_questionnaire_node
can be moved to thenode
class
- Explore moving
ScoredQuestion
initialization into thescored_question.rb
model.
GitHub links and Pull Request
Link to Expertiza repository: here
Link to the forked repository: here
Link to Pull Request: here