CSC/ECE 517 Spring 2022 - E2217: Refactor questionnaires controller: Difference between revisions
No edit summary |
No edit summary |
||
(50 intermediate revisions by 3 users not shown) | |||
Line 2: | Line 2: | ||
== Description == | == Description == | ||
This project focused on sanding down | 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 == | |||
===='''Problem''': The <code>create_questionnaire</code> method is not used==== | |||
<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==== | |||
*'''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]] | |||
[[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. | |||
Addition of constants to <code>scored_question.rb</code>: | |||
[[File:Scored_question_changes.png]] | |||
===='''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 | |||
Change of <code>questionnaire_id</code> to <code>params[:id]</code> in <code>save_all_questions</code>: | |||
[[File:save_all_questions_changes.png]] | |||
===='''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. | |||
[[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 | |||
<code>question.max_label = ScoredQuestion::DEFAULT_MAX_AGREEMENT</code><code>question.min_label = ScoredQuestion::DEFAULT_MIN_AGREEMENT</code> | |||
[[File:score.png]] | |||
== Modified Files == | == Modified Files == | ||
<code>questionnaires_controller.rb</code> | |||
<code>scored_question.rb</code> | |||
<code>questionnaires_controller_spec.rb</code> | |||
== Testing == | == Testing == | ||
Line 15: | Line 61: | ||
</pre> | </pre> | ||
==Pull Request== | ==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