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.

Before Refactoring: survey_responses_controller.rb

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

After Refactoring: survey_responses_controller.rb

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

Code Clean up

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

Testing

On VCL session there are 2 instances running. On port 3000 of host http://152.46.18.142/ the original instance is setup and on port 3000 of host http://152.46.19.10:3000/ the refactored instance is setup. 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

View of Re-factored Course Evaluation Survey	View of Original Course Evaluation Survey

View of Re-factored View Responses	View of Original View Responses

View of Re-factored Pending Course Evaluations	View of Original Pending Course Evaluations

View of Response Comments Re-factored	View of Original Response Comments Re-factored