Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2014

2014-11-11T22:35:53Z

Atripat5:

*[[CSC/ECE_517_Fall_2014/sample_page]]
*[[CSC/ECE_517_Fall_2014/ch1a 22 as]]
*[[CSC/ECE 517 Fall 2014/ch1a 19 mx]]
*[[CSC/ECE 517 Fall 2014/ch1a 3 zq]]
*[[CSC/ECE 517 Fall 2014/ch1a 4 lf]]
*[[CSC/ECE 517 Fall 2014/ch1a 4 wl]]
*[[CSC/ECE 517 Fall 2014/ch1a a7 ch]]
*[[CSC/ECE 517 Fall 2014/ch1a 25 rs]]
*[[CSC/ECE 517 Fall 2014/ch1a 25 jf]]
*[[CSC/ECE 517 Fall 2014/ch1a 8 os]]
*[[CSC/ECE 517 Fall 2014/ch1a 8 sn]]
*[[CSC/ECE 517 Fall 2014/ch1a 15 gs]]
*[[CSC/ECE 517 Fall 2014/ch1a 10 hu]]
*[[CSC/ECE 517 Fall 2014/ch1a 20 kv]]
*[[CSC/ECE 517 Fall 2014/ch1a 21 as]]
*[[CSC/ECE 517 Fall 2014/ch1a 24 sa]]
*[[CSC/ECE 517 Fall 2014/ch1a 26 sn]]
*[[CSC/ECE 517 Fall 2014/ch1a 6 rl]]
*[[CSC/ECE 517 Fall 2014/ch1a 2 ss]]
*[[CSC/ECE 517 Fall 2014/ch1a 16 av]]
*[[CSC/ECE 517 Fall 2014/ch1a 1 rm]]
*[[CSC/ECE 517 Fall 2014/ch1a 1 sj]]
*[[CSC/ECE 517 Fall 2014/ch1a 23 ss]]
*[[CSC/ECE 517 Fall 2014/ch1a 20 rn]]
*[[CSC/ECE 517 Fall 2014/ch1a 22 sp]]
*[[CSC/ECE 517 Fall 2014/oss M1454 rss]]

*[[CSC/ECE 517 Fall 2014/ch1a 26 gn]]

*[[CSC/ECE 517 Fall 2014/ch1a 13 va]]
*[[CSC/ECE 517 Fall 2014/ch1a 9 aa]]
*[[CSC/ECE 517 Fall 2014/ch1a 9 kn]]
*[[CSC/ECE 517 Fall 2014/ch1a 11 ap]]
*[[CSC/ECE 517 Fall 2014/ch1a 25 ks]]
*[[CSC/ECE 517 Fall 2014/ch1a 7 kz]]
*[[CSC/ECE_517_Fall_2014/ch1a_6_bn]]
*[[CSC/ECE_517_Fall_2014/ch1a 10 zz]]
*[[CSC/ECE_517_Fall_2014/ch1a 16 va]]
*[[CSC/ECE_517_Fall_2014/ch1a F1415 rv]]
*[[CSC/ECE_517_Fall_2014/ch1a_3_cp]]
*[[CSC/ECE 517 Fall 2014/ch1b 26 sa]]
*[[CSC/ECE_517_Fall_2014/ch1b_28_cg]]
*[[CSC/ECE_517_Fall_2014/ch1b 29 ry]]
*[[CSC/ECE_517_Fall_2014/ch1b 30 cs]]
*[[CSC/ECE_517_Fall_2014/ch1b_33_jy]]
*[[CSC/ECE_517_Fall_2014/ch1b_27_js]]
*[[CSC/ECE 517 Fall 2014/oss E1453 syy]]
*[[CSC/ECE 517 Fall 2014/oss E1463 vpd]]
*[[CSC/ECE 517 Fall 2014/oss E1465 oak]]
*[[CSC/ECE 517 Fall 2014/oss_M1456_kdv]]
*[[CSC/ECE 517 Fall 2014/oss_M1453_sst]]
*[[CSC/ECE 517 Fall 2014/oss_E1456_akk]]
*[[CSC/ECE_517_Fall_2014/oss_M1455_asa]]
*[[CSC/ECE_517_Fall_2014/oss_E1458_sst]]
*[[CSC/ECE_517_Fall_2014/oss_E1457_ags]]
*[[CSC/ECE_517_Fall_2014/OSS_E1466_gjf]]
*[[CSC/ECE_517_Fall_2014/oss_M1452_jns]]
*[[CSC/ECE_517_Fall_2014/oss_E1462_nms]]
*[[CSC/ECE_517_Fall_2014/OSS_S1455_ajp]]
*[[CSC/ECE_517_Fall_2014/OSS_S1454_ccc]]
*[[CSC/ECE_517_Fall_2014/OSS_E1450_cxm]]
*[[CSC/ECE_517_Fall_2014/OSS_E1455_skn]]
*[[CSC/ECE_517_Fall_2014/OSS_M1450_vda]]
*[[CSC/ECE_517_Fall_2014/OSS_E1467_rsv]]
*[[CSC/ECE_517_Fall_2014/OSS_E1464_vnn]]
*[[CSC/ECE 517 Fall 2014/OSS_M1451_ahs]]
*[[CSC/ECE 517 Fall 2014/OSS_E1451_las]]
*[[CSC/ECE 517 Fall 2014/OSS_E1461_knn]]
*[[CSC/ECE_517_Fall_2014/oss_E1460_aua]]
*[[CSC/ECE_517_Fall_2014/oss_E1459_jjr]]
*[[CSC/ECE_517_Fall_2014/final_E1472_gjfz]]
*[[CSC/ECE_517_Fall_2014/final_design_doc_M1451_hsss]]
*[[CSC/ECE_517_Fall_2014/final_E1471_asuv]]
*[[CSC/ECE_517_Fall_2014/final_E1475_nrnn]]
*[[CSC/ECE_517_Fall_2014/final_design_doc_M1450_navr]]

CSC/ECE 517 Fall 2014

2014-11-10T22:45:23Z

Atripat5:

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-30T03:33:12Z

Atripat5: /* Running Project & Testing Functionality */

== '''E1456 Refactoring Questionnaire Controller''' ==

__TOC__
[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit# Problem_Statement] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit# Global Rules]

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller. 

Why is Refactoring Required
*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=
The scope of the project is to refactor Questionnaire Controller which is very huge, in order to follow the standard principle of Fat Models and slim controllers.

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Refactoring questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|''''' 2 '''''
|edit
|Three functions changed in this method
|Refactoring using Global rules
|-
|''''' 3 '''''
|create_questionnaire
|Removal of redundant If statement
|Removal of unnecessary statements
|-
|''''' 4 '''''
|create_questionnaire
|Commenting out unnecessary print statements
|Refactoring of the function
|-
|''''' 5 '''''
|create
|statement change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 6 '''''
|update
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 7 '''''
|toggle_access
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 8 '''''
|save
|change from QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id) to QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
|Refactoring using Global rules
|-
|''''' 9 '''''
|save_new_questions(questionnaire_id)
|change from a = QuestionAdvice.new(:score => i, :advice => nil) to a = QuestionAdvice.new(score: i, advice: nil)
|Refactoring using Global rules
|-
|''''' 10 '''''
|save_questions(questionnaire_id)
|Deletion of an extra If statement and deletion of an unnecessary print statement
|Refactoring was required for this function
|-
|''''' 11 '''''
|save_choices(questionnaire_id)
|Removal of unnecessary print statements and refactoring of the function
|Refactoring done following Global rules and use of DRY principle
|-
|''''' 12 '''''
|export
|Refactoring done for 2 statements
|Refactoring done using Global rules
|-
|''''' 13 '''''
|clone_questionnaire_details(questions)
|Refactoring of 4 statements done in this function
|Refactoring was required for this function
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

<pre>
def select_questionnaire_type
@questionnaire = Object.const_get(params[:questionnaire][:type]).new(params[:questionnaire])
@questionnaire.private = params[:questionnaire][:private]
@questionnaire.min_question_score = params[:questionnaire][:min_question_score]
@questionnaire.max_question_score = params[:questionnaire][:max_question_score]
@questionnaire.section = params[:questionnaire][:section]
@questionnaire.id = params[:questionnaire][:id]
@questionnaire.display_type = params[:questionnaire][:display_type]
end
</pre>

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......
</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......
</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........
</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv"
........
</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........
</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

8. Removed unnecessary debug prints present in the code.
<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

9. Used `&&` and `||` rather than `and` and `or` to keep boolean precedence as per global rules.

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

10. More global rule changes.
<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Running Project & Testing Functionality=
We tested the functionality of the questionnaires controller before and after refactoring by manually creating and listing the questionnaires, there is no change in behavior observed. Please find the snapshots below for reference.
After Refactoring, the questionnaire controller is working as before.
[[File:rsz_questionnaires_index1.jpg|100px|frame|center]]

[[File:rsz_questionnaires_review.jpg|frame|center]]

[[File:rsz_questionnaires_teammate_review.jpg|frame|center]]

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-30T03:32:56Z

Atripat5: /* Testing Functionality */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-30T03:31:53Z

Atripat5: /* Testing Functionality */

== '''E1456 Refactoring Questionnaire Controller''' ==

__TOC__
[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit# Problem_Statement] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit# Global Rules]

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller. 

Why is Refactoring Required
*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=
The scope of the project is to refactor Questionnaire Controller which is very huge, in order to follow the standard principle of Fat Models and slim controllers.

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Refactoring questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|''''' 2 '''''
|edit
|Three functions changed in this method
|Refactoring using Global rules
|-
|''''' 3 '''''
|create_questionnaire
|Removal of redundant If statement
|Removal of unnecessary statements
|-
|''''' 4 '''''
|create_questionnaire
|Commenting out unnecessary print statements
|Refactoring of the function
|-
|''''' 5 '''''
|create
|statement change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 6 '''''
|update
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 7 '''''
|toggle_access
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 8 '''''
|save
|change from QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id) to QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
|Refactoring using Global rules
|-
|''''' 9 '''''
|save_new_questions(questionnaire_id)
|change from a = QuestionAdvice.new(:score => i, :advice => nil) to a = QuestionAdvice.new(score: i, advice: nil)
|Refactoring using Global rules
|-
|''''' 10 '''''
|save_questions(questionnaire_id)
|Deletion of an extra If statement and deletion of an unnecessary print statement
|Refactoring was required for this function
|-
|''''' 11 '''''
|save_choices(questionnaire_id)
|Removal of unnecessary print statements and refactoring of the function
|Refactoring done following Global rules and use of DRY principle
|-
|''''' 12 '''''
|export
|Refactoring done for 2 statements
|Refactoring done using Global rules
|-
|''''' 13 '''''
|clone_questionnaire_details(questions)
|Refactoring of 4 statements done in this function
|Refactoring was required for this function
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

<pre>
def select_questionnaire_type
@questionnaire = Object.const_get(params[:questionnaire][:type]).new(params[:questionnaire])
@questionnaire.private = params[:questionnaire][:private]
@questionnaire.min_question_score = params[:questionnaire][:min_question_score]
@questionnaire.max_question_score = params[:questionnaire][:max_question_score]
@questionnaire.section = params[:questionnaire][:section]
@questionnaire.id = params[:questionnaire][:id]
@questionnaire.display_type = params[:questionnaire][:display_type]
end
</pre>

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......
</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......
</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........
</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv"
........
</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........
</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

8. Removed unnecessary debug prints present in the code.
<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

9. Used `&&` and `||` rather than `and` and `or` to keep boolean precedence as per global rules.

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

10. More global rule changes.
<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Testing Functionality=
We tested the functionality of the questionnaires controller before and after refactoring by manually creating and listing the questionnaires, there is no change in behavior observed. Please find the snapshots below for reference.
After Refactoring, the questionnaire controller is working as before.
[[File:rsz_questionnaires_index1.jpg|100px|frame|center]]
[[File:rsz_questionnaires_review.jpg|frame|center]]
[[File:rsz_questionnaires_teammate_review.jpg|frame|center]]

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-30T03:29:59Z

Atripat5: /* Testing Functionality */

== '''E1456 Refactoring Questionnaire Controller''' ==

__TOC__
[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit# Problem_Statement] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit# Global Rules]

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller. 

Why is Refactoring Required
*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=
The scope of the project is to refactor Questionnaire Controller which is very huge, in order to follow the standard principle of Fat Models and slim controllers.

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Refactoring questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|''''' 2 '''''
|edit
|Three functions changed in this method
|Refactoring using Global rules
|-
|''''' 3 '''''
|create_questionnaire
|Removal of redundant If statement
|Removal of unnecessary statements
|-
|''''' 4 '''''
|create_questionnaire
|Commenting out unnecessary print statements
|Refactoring of the function
|-
|''''' 5 '''''
|create
|statement change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 6 '''''
|update
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 7 '''''
|toggle_access
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 8 '''''
|save
|change from QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id) to QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
|Refactoring using Global rules
|-
|''''' 9 '''''
|save_new_questions(questionnaire_id)
|change from a = QuestionAdvice.new(:score => i, :advice => nil) to a = QuestionAdvice.new(score: i, advice: nil)
|Refactoring using Global rules
|-
|''''' 10 '''''
|save_questions(questionnaire_id)
|Deletion of an extra If statement and deletion of an unnecessary print statement
|Refactoring was required for this function
|-
|''''' 11 '''''
|save_choices(questionnaire_id)
|Removal of unnecessary print statements and refactoring of the function
|Refactoring done following Global rules and use of DRY principle
|-
|''''' 12 '''''
|export
|Refactoring done for 2 statements
|Refactoring done using Global rules
|-
|''''' 13 '''''
|clone_questionnaire_details(questions)
|Refactoring of 4 statements done in this function
|Refactoring was required for this function
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

<pre>
def select_questionnaire_type
@questionnaire = Object.const_get(params[:questionnaire][:type]).new(params[:questionnaire])
@questionnaire.private = params[:questionnaire][:private]
@questionnaire.min_question_score = params[:questionnaire][:min_question_score]
@questionnaire.max_question_score = params[:questionnaire][:max_question_score]
@questionnaire.section = params[:questionnaire][:section]
@questionnaire.id = params[:questionnaire][:id]
@questionnaire.display_type = params[:questionnaire][:display_type]
end
</pre>

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......
</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......
</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........
</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv"
........
</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........
</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

8. Removed unnecessary debug prints present in the code.
<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

9. Used `&&` and `||` rather than `and` and `or` to keep boolean precedence as per global rules.

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

10. More global rule changes.
<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Testing Functionality=
We tested the functionality of the questionnaires controller before and after refactoring by manually creating and listing the questionnaires, there is no change in behavior observed. Please find the snapshots below for reference.
After Refactoring, the questionnaire controller is working as before.
[[File:questionnaires_index.JPG|100px|frame|left]]
[[File:questionnaires_review.JPG|frame|left]]
[[File:questionnaires_teammate_review.JPG|frame|left]]

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-30T03:28:57Z

Atripat5: /* Testing Functionality */

== '''E1456 Refactoring Questionnaire Controller''' ==

__TOC__
[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit# Problem_Statement] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit# Global Rules]

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller. 

Why is Refactoring Required
*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=
The scope of the project is to refactor Questionnaire Controller which is very huge, in order to follow the standard principle of Fat Models and slim controllers.

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Refactoring questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|''''' 2 '''''
|edit
|Three functions changed in this method
|Refactoring using Global rules
|-
|''''' 3 '''''
|create_questionnaire
|Removal of redundant If statement
|Removal of unnecessary statements
|-
|''''' 4 '''''
|create_questionnaire
|Commenting out unnecessary print statements
|Refactoring of the function
|-
|''''' 5 '''''
|create
|statement change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 6 '''''
|update
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 7 '''''
|toggle_access
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 8 '''''
|save
|change from QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id) to QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
|Refactoring using Global rules
|-
|''''' 9 '''''
|save_new_questions(questionnaire_id)
|change from a = QuestionAdvice.new(:score => i, :advice => nil) to a = QuestionAdvice.new(score: i, advice: nil)
|Refactoring using Global rules
|-
|''''' 10 '''''
|save_questions(questionnaire_id)
|Deletion of an extra If statement and deletion of an unnecessary print statement
|Refactoring was required for this function
|-
|''''' 11 '''''
|save_choices(questionnaire_id)
|Removal of unnecessary print statements and refactoring of the function
|Refactoring done following Global rules and use of DRY principle
|-
|''''' 12 '''''
|export
|Refactoring done for 2 statements
|Refactoring done using Global rules
|-
|''''' 13 '''''
|clone_questionnaire_details(questions)
|Refactoring of 4 statements done in this function
|Refactoring was required for this function
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

<pre>
def select_questionnaire_type
@questionnaire = Object.const_get(params[:questionnaire][:type]).new(params[:questionnaire])
@questionnaire.private = params[:questionnaire][:private]
@questionnaire.min_question_score = params[:questionnaire][:min_question_score]
@questionnaire.max_question_score = params[:questionnaire][:max_question_score]
@questionnaire.section = params[:questionnaire][:section]
@questionnaire.id = params[:questionnaire][:id]
@questionnaire.display_type = params[:questionnaire][:display_type]
end
</pre>

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......
</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......
</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........
</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv"
........
</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........
</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

8. Removed unnecessary debug prints present in the code.
<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

9. Used `&&` and `||` rather than `and` and `or` to keep boolean precedence as per global rules.

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

10. More global rule changes.
<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Testing Functionality=
We tested the functionality of the questionnaires controller before and after refactoring by manually creating and listing the questionnaires, there is no change in behavior observed. Please find the snapshots below for reference.
After Refactoring, the questionnaire controller is working as before.
[[File:questionnaires_index1.JPG|100px|frame|left]]
[[File:questionnaires_review.JPG|frame|left]]
[[File:questionnaires_teammate_review.JPG|frame|left]]

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-30T03:28:22Z

Atripat5: /* Testing Functionality */

== '''E1456 Refactoring Questionnaire Controller''' ==

__TOC__
[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit# Problem_Statement] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit# Global Rules]

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller. 

Why is Refactoring Required
*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=
The scope of the project is to refactor Questionnaire Controller which is very huge, in order to follow the standard principle of Fat Models and slim controllers.

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Refactoring questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|''''' 2 '''''
|edit
|Three functions changed in this method
|Refactoring using Global rules
|-
|''''' 3 '''''
|create_questionnaire
|Removal of redundant If statement
|Removal of unnecessary statements
|-
|''''' 4 '''''
|create_questionnaire
|Commenting out unnecessary print statements
|Refactoring of the function
|-
|''''' 5 '''''
|create
|statement change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 6 '''''
|update
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 7 '''''
|toggle_access
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 8 '''''
|save
|change from QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id) to QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
|Refactoring using Global rules
|-
|''''' 9 '''''
|save_new_questions(questionnaire_id)
|change from a = QuestionAdvice.new(:score => i, :advice => nil) to a = QuestionAdvice.new(score: i, advice: nil)
|Refactoring using Global rules
|-
|''''' 10 '''''
|save_questions(questionnaire_id)
|Deletion of an extra If statement and deletion of an unnecessary print statement
|Refactoring was required for this function
|-
|''''' 11 '''''
|save_choices(questionnaire_id)
|Removal of unnecessary print statements and refactoring of the function
|Refactoring done following Global rules and use of DRY principle
|-
|''''' 12 '''''
|export
|Refactoring done for 2 statements
|Refactoring done using Global rules
|-
|''''' 13 '''''
|clone_questionnaire_details(questions)
|Refactoring of 4 statements done in this function
|Refactoring was required for this function
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

<pre>
def select_questionnaire_type
@questionnaire = Object.const_get(params[:questionnaire][:type]).new(params[:questionnaire])
@questionnaire.private = params[:questionnaire][:private]
@questionnaire.min_question_score = params[:questionnaire][:min_question_score]
@questionnaire.max_question_score = params[:questionnaire][:max_question_score]
@questionnaire.section = params[:questionnaire][:section]
@questionnaire.id = params[:questionnaire][:id]
@questionnaire.display_type = params[:questionnaire][:display_type]
end
</pre>

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......
</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......
</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........
</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv"
........
</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........
</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

8. Removed unnecessary debug prints present in the code.
<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

9. Used `&&` and `||` rather than `and` and `or` to keep boolean precedence as per global rules.

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

10. More global rule changes.
<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Testing Functionality=
We tested the functionality of the questionnaires controller before and after refactoring by manually creating and listing the questionnaires, there is no change in behavior observed. Please find the snapshots below for reference.
After Refactoring, the questionnaire controller is working as before.
[[File:rsz_questionnaires_index1.JPG|100px|frame|left]]
[[File:rsz_questionnaires_review.JPG|frame|left]]
[[File:rsz_questionnaires_teammate_review.JPG|frame|left]]

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

File:Rsz questionnaires teammate review.jpg

2014-10-30T03:27:07Z

Atripat5:

File:Rsz questionnaires review.jpg

2014-10-30T03:26:53Z

Atripat5:

File:Rsz questionnaires index1.jpg

2014-10-30T03:26:36Z

Atripat5:

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:53:22Z

Atripat5: /* Code Snippets */

== '''E1456 Refactoring Questionnaire Controller''' ==

__TOC__
[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit# Problem_Statement] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit# Global Rules]

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller. 

Why is Refactoring Required
*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=
The scope of the project is to refactor Questionnaire Controller which is very huge, in order to follow the standard principle of Fat Models and slim controllers.

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Refactoring questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|''''' 2 '''''
|edit
|Three functions changed in this method
|Refactoring using Global rules
|-
|''''' 3 '''''
|create_questionnaire
|Removal of redundant If statement
|Removal of unnecessary statements
|-
|''''' 4 '''''
|create_questionnaire
|Commenting out unnecessary print statements
|Refactoring of the function
|-
|''''' 5 '''''
|create
|statement change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 6 '''''
|update
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 7 '''''
|toggle_access
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 8 '''''
|save
|change from QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id) to QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
|Refactoring using Global rules
|-
|''''' 9 '''''
|save_new_questions(questionnaire_id)
|change from a = QuestionAdvice.new(:score => i, :advice => nil) to a = QuestionAdvice.new(score: i, advice: nil)
|Refactoring using Global rules
|-
|''''' 10 '''''
|save_questions(questionnaire_id)
|Deletion of an extra If statement and deletion of an unnecessary print statement
|Refactoring was required for this function
|-
|''''' 11 '''''
|save_choices(questionnaire_id)
|Removal of unnecessary print statements and refactoring of the function
|Refactoring done following Global rules and use of DRY principle
|-
|''''' 12 '''''
|export
|Refactoring done for 2 statements
|Refactoring done using Global rules
|-
|''''' 13 '''''
|clone_questionnaire_details(questions)
|Refactoring of 4 statements done in this function
|Refactoring was required for this function
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

<pre>
def select_questionnaire_type
@questionnaire = Object.const_get(params[:questionnaire][:type]).new(params[:questionnaire])
@questionnaire.private = params[:questionnaire][:private]
@questionnaire.min_question_score = params[:questionnaire][:min_question_score]
@questionnaire.max_question_score = params[:questionnaire][:max_question_score]
@questionnaire.section = params[:questionnaire][:section]
@questionnaire.id = params[:questionnaire][:id]
@questionnaire.display_type = params[:questionnaire][:display_type]
end
</pre>

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......
</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......
</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........
</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv"
........
</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........
</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

8. Removed unnecessary debug prints present in the code.
<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

9. Used `&&` and `||` rather than `and` and `or` to keep boolean precedence as per global rules.

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

10. More global rule changes.
<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:52:23Z

Atripat5: /* Code Snippets */

== '''E1456 Refactoring Questionnaire Controller''' ==

__TOC__
[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit# Problem_Statement] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit# Global Rules]

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller. 

Why is Refactoring Required
*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=
The scope of the project is to refactor Questionnaire Controller which is very huge, in order to follow the standard principle of Fat Models and slim controllers.

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Refactoring questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|''''' 2 '''''
|edit
|Three functions changed in this method
|Refactoring using Global rules
|-
|''''' 3 '''''
|create_questionnaire
|Removal of redundant If statement
|Removal of unnecessary statements
|-
|''''' 4 '''''
|create_questionnaire
|Commenting out unnecessary print statements
|Refactoring of the function
|-
|''''' 5 '''''
|create
|statement change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 6 '''''
|update
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 7 '''''
|toggle_access
|change from redirect_to :controller => 'tree_display', :action => 'list' to redirect_to controller: 'tree_display', action: 'list'
|Refactoring using Global rules
|-
|''''' 8 '''''
|save
|change from QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id) to QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
|Refactoring using Global rules
|-
|''''' 9 '''''
|save_new_questions(questionnaire_id)
|change from a = QuestionAdvice.new(:score => i, :advice => nil) to a = QuestionAdvice.new(score: i, advice: nil)
|Refactoring using Global rules
|-
|''''' 10 '''''
|save_questions(questionnaire_id)
|Deletion of an extra If statement and deletion of an unnecessary print statement
|Refactoring was required for this function
|-
|''''' 11 '''''
|save_choices(questionnaire_id)
|Removal of unnecessary print statements and refactoring of the function
|Refactoring done following Global rules and use of DRY principle
|-
|''''' 12 '''''
|export
|Refactoring done for 2 statements
|Refactoring done using Global rules
|-
|''''' 13 '''''
|clone_questionnaire_details(questions)
|Refactoring of 4 statements done in this function
|Refactoring was required for this function
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

<pre>
def select_questionnaire_type
@questionnaire = Object.const_get(params[:questionnaire][:type]).new(params[:questionnaire])
@questionnaire.private = params[:questionnaire][:private]
@questionnaire.min_question_score = params[:questionnaire][:min_question_score]
@questionnaire.max_question_score = params[:questionnaire][:max_question_score]
@questionnaire.section = params[:questionnaire][:section]
@questionnaire.id = params[:questionnaire][:id]
@questionnaire.display_type = params[:questionnaire][:display_type]
end
</pre>

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......
</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......
</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........
</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv"
........
</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........
</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

8. Removed unnecessary debug prints present in the code.
<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

9. Used `&&` and `||` rather than `and` and `or` to keep boolean precedence as per global rules.

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

10. More global rule changes.
<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:37:10Z

Atripat5: /* Introduction */

== '''E1456 Refactoring Questionnaire Controller''' ==

__TOC__
[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit# Problem_Statement] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit# Global Rules]

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller. 

Why is Refactoring Required
*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Questionnaire Controller: questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......
</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......
</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........
</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv"
........
</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........
</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

8. Removed unnecessary debug prints present in the code.
<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

9. Used `&&` and `||` rather than `and` and `or` to keep boolean precedence as per global rules.

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

10. More global rule changes.
<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:36:57Z

Atripat5: /* Introduction */

== '''E1456 Refactoring Questionnaire Controller''' ==

__TOC__
[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit# Problem_Statement] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit# Global Rules]

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller. 
Why is Refactoring Required

*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Questionnaire Controller: questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......
</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......
</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........
</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv"
........
</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........
</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

8. Removed unnecessary debug prints present in the code.
<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

9. Used `&&` and `||` rather than `and` and `or` to keep boolean precedence as per global rules.

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

10. More global rule changes.
<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:36:23Z

Atripat5: /* Why is Refactoring Required */

== '''E1456 Refactoring Questionnaire Controller''' ==

__TOC__
[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit# Problem_Statement] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit# Global Rules]

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller.

=Project Overview=

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Questionnaire Controller: questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......
</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......
</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........
</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........
</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv"
........
</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........
</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

8. Removed unnecessary debug prints present in the code.
<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

9. Used `&&` and `||` rather than `and` and `or` to keep boolean precedence as per global rules.

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

10. More global rule changes.
<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:31:00Z

Atripat5: /* Object Oriented Design Principles Followed */

== '''Refactoring Questionnaire Controller''' ==

__TOC__

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller.

=Why is Refactoring Required=

*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Questionnaire Controller: questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........

</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......

</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......

</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........

</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........

</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)

</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv
........

</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........

</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:26:29Z

Atripat5: /* Object Oriented Design Principles Followed */

== '''Refactoring Questionnaire Controller''' ==

__TOC__

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller.

=Why is Refactoring Required=

*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Questionnaire Controller: questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........

</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......

</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......

</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........

</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........

</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)

</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv
........

</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........

</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:26:18Z

Atripat5: /* Object Oriented Design Principles Followed */

== '''Refactoring Questionnaire Controller''' ==

__TOC__

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller.

=Why is Refactoring Required=

*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Questionnaire Controller: questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........

</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......

</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......

</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........

</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........

</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)

</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv
........

</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........

</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
 
*DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods.

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:25:53Z

Atripat5: /* Object Oriented */

== '''Refactoring Questionnaire Controller''' ==

__TOC__

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller.

=Why is Refactoring Required=

*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Questionnaire Controller: questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb. 

4. save_advice has been moved to the advice_controller. 

5. Global rule changes for using Hash - value key pair implemented [Use key: ‘value’, not :key => ‘value’] accordingly.
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

6. ".eql? nil" used instead of " == nil".
<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........

</pre>

7. Removed duplicate and redundant code making the code DRY.
<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......

</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......

</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........

</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........

</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)

</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv
........

</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........

</pre>

=Object Oriented Design Principles Followed=

The following object oriented design principles were followed during refactoring: 
*Single Responsibility Principle: The single responsibility principle states that every context (class, function, variable, etc.) should have a single responsibility. It was maintained that each method should be involved with a single responsibility and the code that was not related to that particular functionality was moved to other method. This was also taken care of in terms of classes.
 
DRY Principle: Don't Repeat Yourself! The repetitive and redundant code was removed from the associated classes and methods

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:19:04Z

Atripat5: /* References */

== '''Refactoring Questionnaire Controller''' ==

__TOC__

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller.

=Why is Refactoring Required=

*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Questionnaire Controller: questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb

4. save_advice has been moved to the advice_controller.

5. Global rule changes for using Hash - value key pair
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........

</pre>

<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......

</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......

</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........

</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........

</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)

</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv
........

</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........

</pre>

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor] 
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:18:46Z

Atripat5: /* References */

== '''Refactoring Questionnaire Controller''' ==

__TOC__

=Introduction=
Questionnaire Controller interacts with the user to create and edit questionnaires such as review rubrics, teammate-feedback rubrics, quizzes, and surveys. This page provides a detailed description of Open Source Project on Expertiza for refactoring the questionnaire controller.

=Why is Refactoring Required=

*It helps in making the code more understandable
*It makes the code more maintainable
*To remove repetition of code

=Project Overview=

==Classes involved==
*questionnaire_controller.rb
*questionnaire_helper.rb
*advice_controller.rb

==Changes made==
*Functionality moved to quiz_questionnaire.rb.
*edit_advice method was not being used, so it was removed.
*save_advice moved to the advice_controller.
*copy, update_quiz, valid_quiz methods were long and have been broken up. clone_questionnaire_details was also broken up and renamed.
*Added comments to select_questionnaire_type
*Debug output (print statements) have been removed.
*Changed code to follow the global rules.
*save_new_questions, delete_questions, save_questions have been moved to a separate class.

==Project Resources==
[https://github.com/kbhupal/expertiza GitHub Project Repository]

[http://152.1.13.221:2555/ VCL IP Address]

[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]

=UML CLass Diagram=
A Subset of the UML class diagram including only the related classes for this project can be drawn as shown below.

[[File:UMLDiagram.JPG|frame|center]]

Questionnaire Controller is the superclass of QuizQuestionnaire and all the other classes - User class, Advice class and Questionnaire classes are related model Classes to the controllers that we have modified.

=Questionnaire Controller: questionnaire_controller.rb =

Several methods in Questionnaire Controller class required refactoring to ensure that the change is reflected in the entire project. The names were changed to follow method naming conventions in Ruby. Deprecated code was removed and re-coded to ensure that the code worked in future version of Rails as well. Duplicate codes were commented out. Methods that are more general in the sub classes were moved to the parent class. Some methods that calculate that were colliding with other files were moved to a helper method questionnaire_helper.rb .

{| class="wikitable"
|-
! style="width:5%;"|Sr. No.
! style="width:13%;"|Method Name
! style="width:33%;"|Changes Made
! style="width:43%;"|Reason For Change
|- style="vertical-align:top;"
|''''' 1 '''''
|delete
|redirect_to :action => 'list', :controller => 'tree_display' to redirect_to action: 'list', controller: 'tree_display'
|Refactoring using Global rules
|-
|}

=Code Snippets=
1. The quiz questionnaire related methods such as view_quiz, update_quiz, new_quiz and validate_quiz are moved to questionnaire_helper.rb to make the questionnaires_controller thin and follow global rule of thin controllers and fat models. 
[[File:add_include_line.PNG|frame|center]]

2. Non Restful method names changed accordingly to make them Restful. 

Before

After
[[File:after_refactoring_function_name_change.PNG|frame|center]]

3. edit_advice method is evidently not used, and removed. This edit function is already implemented in advice_controller.rb

4. save_advice has been moved to the advice_controller.

5. Global rule changes for using Hash - value key pair
<pre>
Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........
</pre>

<pre>
Before Refactoring :-
def edit
.........
redirect_to Questionnaire if @questionnaire == nil
.........
redirect_to :action => 'view', :id => @questionnaire
..........
redirect_to :controller => 'advice', :action => 'edit_advice', :id => params[:questionnaire][:id]
..........

After Refactoring :-
def edit
..........
redirect_to Questionnaire if @questionnaire.eql? nil
..........
redirect_to action: 'view', id: @questionnaire
...........
redirect_to controller: 'advice', action: 'edit_advice', id: params[:questionnaire][:id]
..........

</pre>

<pre>
Before Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
if team = Team.find(t.team_id, @assignment.id)
break
end
end

.....
end

After Refactoring Redundant If in def create_questionnaire :-
def create_questionnaire
........
if Team.find(t.team_id, @assignment.id)
#Removed unneeded If statement
team = Team.find(t.team_id, @assignment.id)
break
end
end
</pre>

<pre>
Before Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
print "=====create_questionnaire========="
.....
@successful_create = true
print "=====save in create_questionnaire begin========="
save
print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end

After Refactoring Un-necessary Print statements in def create_questionnaire :-

def create_questionnaire
.......
# print "=====create_questionnaire========="
.......
@successful_create = true
# print "=====save in create_questionnaire begin========="
save
# print "=====save in create_questionnaire over========="
save_choices @questionnaire.id
# print "=====save_choice in create_questionnaire over========="
if @successful_create == true
flash[:note] = "Quiz was successfully created"
end
</pre>

<pre>
Before Refactoring Global rules :-
def create_questionnaire
.......
redirect_to :controller => 'submitted_content', :action => 'edit', :id => participant_id
.......

After Refactoring Global rules :-
def create_questionnaire
.......
redirect_to controller: 'submitted_content', action: 'edit', id: participant_id
.......

</pre>

<pre>
Before Refactoring Global rules :-

def create_questionnaire
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create_questionnaire
.......
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-

def create
.......
redirect_to :controller => 'tree_display', :action => 'list'
.......

After Refactoring Global rules :-

def create
.......
redirect_to controller: 'tree_display', action: 'list'
.......

</pre>

<pre>
Before refactoring Global rules :-

def update
........
redirect_to :controller => 'tree_display', :action => 'list'
........

After Refactoring Global rules :-

def update
........
redirect_to controller: 'tree_display', action: 'list'
.........

</pre>

<pre>
Before Refactoring Global rules :-
def toggle_access
........
redirect_to :controller => 'tree_display', :action => 'list'
end

After Refactoring global rules :-
def toggle_access
........
redirect_to controller: 'tree_display', action: 'list'
........

</pre>

<pre>
Before Refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil and @questionnaire.id > 0
.......
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.......

After refactoring Global rules :-
def save
.......
save_questions @questionnaire.id if @questionnaire.id != nil && @questionnaire.id > 0
.......
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
.......
</pre>

<pre>
Before Refactoring Global rules :-
def save_new_questions(questionnaire_id)
.........
a = QuestionAdvice.new(:score => i, :advice => nil)
........

After refactoring Global rules :-
def save_new_questions(questionnaire_id)
........
a = QuestionAdvice.new(score: i, advice: nil)
.........

</pre>

<pre>
Before Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key,:weight => 1, :txt => params[:question][question_key][:txt] )
else
Question.update(question_key, params[:question][question_key])
end

After Refactoring Extra print statement and deletion of an extra statement:-
def save_questions(questionnaire_id)
...........
# print question_key
..........
if (@questionnaire.type == "QuizQuestionnaire")
Question.update(question_key, weight: 1, txt: params[:question][question_key][:txt] )
end
</pre>

<pre>
Before Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] and params[:new_choices]
...........
print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "True", :iscorrect => "true",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => "False", :iscorrect => "false",:question_id => question.id)
.........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "true",:question_id => question.id)
..........
q = QuizQuestionChoice.new(:txt => params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], :iscorrect => "false",:question_id => question.id)

After Refactoring Global Rules :-
def save_choices(questionnaire_id)
..........
if params[:new_question] && params[:new_choices]
..........
# print "=====choice_key="+choice_key+"======="
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "True", iscorrect: "true", question_id: question.id)
..........
q = QuizQuestionChoice.new(txt: "False", iscorrect: "false", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "true", question_id: question.id)
.........
q = QuizQuestionChoice.new(txt: params[:new_choices][questionnum.to_s][q_type][choice_key][:txt], iscorrect: "false", question_id: question.id)

</pre>

<pre>
Before Refactoring Global rules :-
def export
.........
:type => 'text/csv; charset=iso-8859-1; header=present',
:disposition => "attachment; filename=questionnaires.csv"
..........

After Refactoring Global rules :-
def export
.........
type: 'text/csv; charset=iso-8859-1; header=present',
disposition: "attachment; filename=questionnaires.csv
........

</pre>

<pre>
Before Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire_details(questions)
.........
@questionnaire.name = 'Copy of '+orig_questionnaire.name
.........
QuestionnaireNode.create(:parent_id => parent.id, :node_object_id => @questionnaire.id)
.........
redirect_to :controller => 'questionnaire', :action => 'view', :id => @questionnaire.id
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........

After Refactoring Global rules + Function name change + statement deletion :-
def clone_questionnaire(questions)
.........
#Statement deleted
.........
QuestionnaireNode.create(parent_id: parent.id, node_object_id: @questionnaire.id)
..........
redirect_to controller: 'questionnaire', action: 'view', id: @questionnaire.id
..........
redirect_to action: 'list', controller: 'tree_display'
..........

</pre>

=Conclusion=
*Refactoring was performed as per requirements in the files questionnaire_controller.rb, and advice_controller.rb.
*The code from models cannot be moved directly to controllers, so the code was moved to the helper class questionnaire_helper.rb.
*Some methods with confusing method names have also been renamed.

=References=
[http://expertiza.ncsu.edu/ Expertiza] 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring Wiki] 
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit Global Rules for Refactoring] 
[http://www.agiledeveloper.com/presentations/how_to_approach_refactoring_2up.pdf Why Refactor]
[http://en.wikipedia.org/wiki/Single_responsibility_principle Single Responsibility Principle]

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:13:29Z

Atripat5: /* Why is Refactoring Required */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:06:16Z

Atripat5: /* References */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:05:57Z

Atripat5: /* References */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:05:21Z

Atripat5: /* Introduction */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T23:01:30Z

Atripat5: /* References */

File:Before.JPG

2014-10-29T22:47:52Z

Atripat5: uploaded a new version of "File:Before.JPG"

File:Before.JPG

2014-10-29T22:47:26Z

Atripat5: uploaded a new version of "File:Before.JPG"

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T22:30:22Z

Atripat5: /* Conclusion */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T22:29:44Z

Atripat5: /* Conclusion */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T22:04:55Z

Atripat5: /* UML CLass Diagram */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T22:03:44Z

Atripat5: /* UML CLass Diagram */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T22:03:13Z

Atripat5: /* UML CLass Diagram */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T22:02:07Z

Atripat5: /* UML CLass Diagram */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T22:00:15Z

Atripat5: /* UML CLass Diagram */

File:UMLDiagram.JPG

2014-10-29T21:58:38Z

Atripat5:

File:UMLakk.PNG

2014-10-29T21:55:50Z

Atripat5:

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T21:53:53Z

Atripat5: /* UML CLass Diagram */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T21:51:48Z

Atripat5: /* UML CLass Diagram */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T21:51:29Z

Atripat5: /* UML CLass Diagram */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T21:27:15Z

Atripat5: /* Changes to yaml file */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T21:19:10Z

Atripat5: /* UML CLass Diagram */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T21:16:18Z

Atripat5: /* Project Overview */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T21:16:01Z

Atripat5: /* Project Overview */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T21:15:11Z

Atripat5: /* Project Resources */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T21:14:21Z

Atripat5: /* Project Resources */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-29T21:12:29Z

Atripat5: /* Changes made */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-26T18:49:36Z

Atripat5: /* Classes involved */

CSC/ECE 517 Fall 2014/oss E1456 akk

2014-10-26T18:48:34Z

Atripat5: /* Description */