Expertiza - Refactoring SurveyResponsesController

Introduction

Expertiza<ref name="expertiza>Expertiza http://wikis.lib.ncsu.edu/index.php/Expertiza</ref> is an open source web portal for the use of both students and professors. Expertiza allows us to create reusable learning objects through peer review. It allows project submission, team creation and the submission of almost any document type, including URLs and wiki pages. Students can access their pending and finished assignments, they can do peer reviews on several topics and projects. It is developed on Ruby on Rails platform. More information on Expertiza can be found here. The source code has been forked and cloned for making modifications to the to survey responses controller.

This wiki provides an insight into our contributions to the OSS Expertiza application, focusing on Refactoring the SurveyResponses Controller.

Expertiza project links
Refactored Instance	http://152.46.18.142/:3000	username= "user2" password= "password"
Original Instance	http://152.46.19.10:3000/
Repository link	https://github.com/nmpedema/expertiza <ref>Forked repository</ref>
Pull request	https://github.com/expertiza/expertiza/pull/448 <ref>Pull request</ref>

Refactoring in Ruby

Refactoring helps to<ref>http://www.refactoringinruby.com/</ref>

Understand what led to poor code design
Fix code which is difficult to understand
Express each idea “once and only once”
Recognize missing or inadequately formed classes
Simplify overly complex relationships between classes
Achieve the right balance of responsibilities among objects
Make code easier to test and more maintainable

Code Smells

We identified the following categories of code smells in the helper module:

Fat Classes: No class should be fat. Ever. There is no need for your controllers, models or views to be fat; this shows laziness, as there's been no thought given to the application's design outside of deciding to use a particular framework.

Bad Class names: A good class name is expected to adhere to the Ruby style and design style guidelines. It is expected to convey a reasonable amount of functionality handled.

Lengthy Definitions: These are the methods that have too many responsibilities and collaborators. As a result of which, their overall length has grown too long reducing the maintainability and readability.

Duplicated Code: These are those pieces of code that does almost the same functionality of some other piece of code leading to a bad design style. They are usually too error prone since not everyone identifies all the code responsible for the same task and does the changes whenever the need arises.

Refactoring Techniques

There are many documented refactoring techniques, and a few common ones are below.<ref>http://www.integralist.co.uk/posts/refactoring-techniques/</ref>

Rename Class: Controllers as per Ruby conventions should be plural.
Using Helper classes: No class should be containing lot of code, a better practice is to have all active record queries with in the model and in order to keep as much as Ruby code out of the views, helpers are used. Helpers are the only methods you can access, other than instance methods for an instance you have access to.
Extract Method: It consists of breaking up long methods by shifting overly complex chunks of code into new methods which have very descriptive identifiers.

Project Description

This class creates surveys and records the responses which can be viewed. On submitting the responses, the scores and comments are posted.

Classes : SurveyResponsesController.rb
What it does : Creates surveys, submitting surveys, views responses, posts scores and comments.
What has to be changed :

Pluralize the class SurveyResponseController to SurveyResponsesController
Changing declarations of Arrays and Hashes,removing commented out code
Use of routing helpers instead of hardcoded URLs
Move active record queries to the model or another class
Reducing the number of instance variables per action to one

Modification to Existing Code

Pluralize Controller Class

A new survey_responses_controller.rb file is added : As per Ruby on Rails convention, controller names get pluralized while model names are singular. So, the controller name becomes survey_responses_controller instead of survey_response_controller for SurveyResponse modelclass.

Ruby 1.9 standards for Array and Hashes

Modified declarations of Arrays and Hashes

Before Refactoring : survey_responses_controller.rb

#view_responses
  @responses = Array.new
    
#view_responses
  this_response_survey = Hash.new

After Refactoring :

#view_responses
  @responses = []
    
#view_responses
  this_response_survey = {}

Active Record Queries in Model Class

Controllers are best at parsing the inputs, they call the appropriate models, and then format the outputs. It is desirable to have a skinny controller responsible for parsing inputs and models doing actual validation. A bunch of Active Record queries that existed in survey_responses_controller have been moved to SurveyResponse model.

Before Refactoring: survey_responses_controller.rb

#view_responses
     if !params[:course_eval]
      surveylist = SurveyResponse.where( ["assignment_id = ? and survey_id = ?", params[:id], survey.id])
    else
      surveylist = SurveyResponse.where( ["survey_deployment_id = ? and survey_id = ?", params[:id], survey.id])

After Refactoring: survey_responses_controller.rb SurveyResponse.get_survey_list and SurveyResponse.get_survey_list_with_deploy_id methods have been created in SurveyResponse model for active record operations.

     #view_responses
     if !params[:course_eval]
      survey_list = SurveyResponse.get_survey_list(params[:id], survey.id)
     else
      survey_list = SurveyResponse.get_survey_list_with_deploy_id( params[:id], survey.id)

Reduce Instance Variables

It desirable not to have more than one instance variables in a controller action as it indicates increased coupling. Our goal should be reducing coupling as much as possible and view should have direct access to as few instance variables as possible. Here persist_survey method has been created in the SurveyResponseHelper to minimize instance variables.

Before Refactoring: survey_responses_controller.rb

    for question in @questions
    @new = SurveyResponse.new
    @new.survey_id = @survey_id
    @new.question_id = question.id
    @new.assignment_id = @assignment_id
    @new.survey_deployment_id=params[:survey_deployment_id]
    @new.email = params[:email]
    @new.score = @scores[question.id.to_s]
    @new.comments = @comments[question.id.to_s]
    @new.save
  end

  if !params[:survey_deployment_id]
    @surveys = SurveyHelper::get_assigned_surveys(@assignment_id)
  end

After Refactoring: survey_responses_controller.rb

for question in @questions
    SurveyResponseHelper::persist_survey(@survey_id, question.id, @assignment_id, params[:survey_deployment_id], params[:email])
  end
  if !params[:survey_deployment_id]
    surveys = SurveyHelper::get_assigned_surveys(@assignment_id)
  end

Using Routing Helpers

Creation of a resourceful route will also expose a number of helpers to the controllers in the application. We modified the routes.rb file to include all the actions in the controller.

resources :survey_response do

   collection do
       get :view_responses
       get :begin_survey
       get :comments
   end
end

The resourceful routes generated the helpers given below:

   view_responses_survey_response_index_path  returns /survey_response/view_responses

   begin_survey_survey_response_index_path  returns /survey_response/begin_survey

   comments_survey_response_index_path  returns /survey_response/comments

Code Clean up

Comments and extra spaces have been removed and conventions have been followed to enhance readability of the code.

Testing

Testing was performed by running 2 different VCL instances one with the original code and the other with the re-factored code. While performing the manual testing we noticed that when new course evaluations are created, the survey participants are random users and participants selection is not based on the courses. However, while displaying the pending course evaluations in the course evaluation list view, the courses are taken into consideration. Because of this difference we were not able to submit survey responses and view them. In below images we have shown that the output after refactoring and output without re-factoring for all actions in the SurveyResponsesController.

View of Refactored Begin Survey	View of Original Begin Survey
	Begin Survey action was broken in the original source code, we made fixes to make the functionality work.
View of Re-factored Course Evaluation Survey	View of Original Course Evaluation Survey
	The Course Evaluation action works as expected after refactoring
View of Re-factored View Responses	View of Original View Responses
	The View Response action was broken and fix is made to display the functionality properly.
View of Re-factored Pending Course Evaluations	View of Original Pending Course Evaluations
	Pending course evaluation works as expected after code re-factoring
View of Response Comments Re-factored	View of Original Response Comments Re-factored
	The action for View Response comments was not routed properly, necessary changes have been made to call the appropriate action