CSC/ECE 517 Fall 2014/oss E1456 akk

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

GitHub Project Repository

VCL IP Address

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.

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 .

Sr. No.	Method Name	Changes Made	Reason For Change
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.

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

Before

After

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.

Before Refactoring :-
def delete
.........
redirect_to :action => 'list', :controller => 'tree_display'
.........
After Refactoring :-
def delete
.........
redirect_to action: 'list', controller: 'tree_display'
.........

6. ".eql? nil" used instead of " == nil".

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]
..........

7. Removed duplicate and redundant code making the code DRY.

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

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

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
.......

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'
........

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'
.......

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'
.........

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'
........

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)
.......

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)
.........

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

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)

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
........

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'
..........

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

Expertiza
Code Refactoring Wiki
Global Rules for Refactoring
Why Refactor
Single Responsibility Principle