CSC/ECE 517 Spring 2015/oss E1509 lds

E1509. Refactoring SignUpController and SignUpSheetController

This page talks about an open source project based on Expertiza. As a part of contribution to Expertiza, this project aims to refactor SignUpController and SignUpSheetControllers, which lists topics available for an assignment, allows a user to sign up for topic, checks whether a user is signed up for a topic and so on.

Refactoring

Refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior<ref>Refactoring</ref>.
On refactoring controllers, there are some general principals listed on the project requirements. For example,

• DRY principle. Remove duplicated code as much as possible. And Locate methods in appropriate controllers or model classes.
• Controllers should be written in a RESTful style. Following the REST convention<ref>Refactoring Controller</ref>, the controllers should perform the standard controller actions, using the standard method names as much as possible.

Introduction to Expertiza

Expertiza is a web application developed by Ruby on Rails framework. It serves as a peer review system for professors and students at NC State and some other colleges and universities. Students can submit different assignments and peer-review reusable learning objects (articles, code, web sites, etc)<ref>Expertiza-Github</ref>. It is also a powerful tool for professor to manage courses and assignments and so on.

The Expertiza project is supported by the National Science Foundation. And it is an open source application and the source code can be cloned on Github. Here is our Expertiza project repository on Github. For more details of the Expertiza project, including links to documentation and development, you can search on the Expertiza Wiki page.

Problem Statement

Classes involved

* sign_up_controller.rb 
* sign_up_sheet_controller.rb
* assignments_controller.rb
* sign_up_sheet.rb 
* sign_up_topic.rb 
* signed_up_user.rb
* response.rb

What it does

Lists topics available for an assignment, checks whether a user is signed up for a topic, allows users to sign up for topics.

What's wrong with it

• These two controllers seem to do almost the same thing. They have many of the same methods. SignUpSheetController is much longer, and has many more recent mods, but some methods of SignUpController seem more sophisticated than SignUpSheetController. So, your first job is to figure out if both controllers are being used. If not, remove the unused controller. Or, move the functions to a single controller if that makes sense to do.
• Neither controller is at all RESTful; i.e.., its method names aren’t the standard names new, create, edit, delete, etc. Functionality is divided differently than in a standard controller.

* def confirm_topic(creator_id, topic_id, assignment_id)
* def delete_signup
* def delete_signup_for_topic(assignment_id,topic_id)
* def other_confirmed_topic_for_user(assignment_id, creator_id)
* def signup
* def slotAvailable?(topic_id)

• Functionality that should be in models is incorporated into the controller.
• Some methods are too long and appear to do more than one thing
• This class interfaces with assignments_controller so that a list of topics can be displayed when one is editing an assignment. Please be careful that your changes do not affect the functionality on the Topics tab of editing an assignment.
• Rename the controller(s) to SignupController and/or SignupSheetController. (“Sign up”, which gets written as SignUp in camel case, is a verb, whereas “Signup” is a noun.)
• Some codes does not follow the Global Rules.

Modification We Made

Removing unused controller

After various analysis and test, we found SignUpController is never used. So we remove it. The reasons are listed as follows:

• There is no code about SignUpController in route.rb file.

  The Rails router recognizes URLs and dispatches them to a controller's action. It can also generate paths and URLs, avoiding the need to hardcode strings in your views<ref>RubyOnRails-Route</ref>. Rails default routing is resource routing, which allows us to declare all the common routes for a resourceful controller. In our route.rb, we found a matching resourceful route for SignUpSheetController, which asks the router to map it to the SignUpSheetController action.

  resources :signup_sheet 

This route creates several different routes in the application, all mapping to the SignUpSheetController controller. Here are some examples:

HTTP Verb	Controller#Action	Used For
GET	sigh_up_sheet#signup	This function lets the user choose a particular topic
GET	sigh_up_sheet#delete_signup	This function is used to delete a previous signup
GET	sigh_up_sheet#view_publishing_rights	The function is to make sure the publishing rights for a user

However, there is not a matching route for SignUpController.

• In general, SignUpController provides "Signup" functions for a user (student), e.g., sign up a topic, drop a selected topic. SignUpSheetController provides all related functions not only for a user, but also for a administrator, such as managing a signup sheet for an assignment. We foundSignUpSheetController has all seven methods in SignUpController with same name. We found these functions are completely achieved via SignUpSheetController. And we test and prove that codes in SignUpController never got executed in the following ways:
  
  • Find usages of each method in SignUpController. Right-click on each method's name, and choose "Find Usages", then we can find places calling these methods.
    
  • Writing puts sentences to test each method manually. We can find if the method is executed by checking the log on the Console.

RESTful style implementation

The purpose of the list method in the SignupSheet controller is to list all the topics in the specific assignment. According to RESTful rules, a method that returns a list of all available objects should be named as index. Therefore, we rename the list method to the index method in the controller and in all the files that have references to the list method of the signup_sheet_controller.

Before Refactoring:
Method: list

def list
  ...
end

After Refactoring:
Method: index

def index
  ...
end

And we need to change some references to the method:

verify method: 'post', :only => [:destroy, :create, :update],
:redirect_to => {:action => :index}

SignUpTopic.reassign_topic(@user_id,params[:assignment_id], params[:id])
redirect_to action: 'index', id: params[:assignment_id]

signup_team(@assignment.id, @user_id, params[:id])
redirect_to action: 'index', id: params[:assignment_id]

For some others, it is not good to combine some functions with the standard ones. For example, create function creates topics, which is one of the administrator’s functions. However signup creates a record that includes information for topic and student, which happens when student clicks signup button. We can see that create and signup are designed for different roles and different usages so it is not a good idea to combine them together.

Moving inappropriate functionality from controller to model

The controller connects the model with the view. In Rails, controllers are implemented as ActionController classes. The controller knows how to process the data that comes from the model and how to pass it onto the view. The controller should not include any database related actions (such as modifying data before it gets saved inside the database). This should be handled in the proper model. So we move all the database related functionality in the signup_sheet_controller to related models. The following files were modified:

app/controllers/signup_sheet_controller.rb
app/models/deadline_type.rb
app/models/due_date.rb
app/models/participant.rb
app/models/sign_up_topic.rb
app/models/signed_up_user.rb
app/models/topic_deadline.rb
app/models/topic_dependency.rb

Here are two examples on how these files changed.
1. replace "where" in the controller with a new method in related model.
Before Refactoring:
In signup_sheet_controller.rb:

 topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id:  params[:id]).first

After Refactoring:
In signup_sheet_controller.rb:

 topic = SignUpTopic.find_with_name_and_assignment_id(params[:topic][:topic_name], params[:id]).first

Also add a related method in sign_up_topic.rb

def find_with_name_and_assignment_id(name, aid)
 SignUpTopic.where(topic_name: name, assignment_id: aid)
end

2. replace "find_by_sql" in the controller with a new method in related model.
Before Refactoring:
In signup_sheet_controller.rb:

 @duedates = SignUpTopic.find_by_sql("SELECT s.id as topic_id FROM sign_up_topics s WHERE s.assignment_id = " + params[:id].to_s)

After Refactoring:
In signup_sheet_controller.rb:

 @duedates = SignUpTopic.find_topic_id_with_assignment_id(params[:id].to_s)

Also add a related method in sign_up_topic.rb

def find_topic_id_with_assignment_id(aid)
 SignUpTopic.find_by_sql("SELECT s.id as topic_id FROM sign_up_topics s WHERE s.assignment_id = " + aid)
end

Breaking down long methods

We’ve find that lots of methods do more than one thing so we separate a long function into several concise functions and call them in the original function. Here is an example:

There are some preparation works and check works before or after the actual create operation. So we separate two functions update_topic_info and check_after_create from create. And we call these two functions in create method instead of executing these codes directly.

Before Refactoring:
Method: create

def create
  topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id:  params[:id]).first
  if topic != nil
    topic.topic_identifier = params[:topic][:topic_identifier]
    if SignedUpUser.find_by_topic_id(topic.id).nil? || topic.max_choosers == params[:topic][:max_choosers]
      topic.max_choosers = params[:topic][:max_choosers]
    else
      if topic.max_choosers.to_i < params[:topic][:max_choosers].to_i
        topic.update_waitlisted_users(params[:topic][:max_choosers])
        topic.max_choosers = params[:topic][:max_choosers]
      else
        flash[:error] = 'Value of maximum choosers can only be increased! No change has been made to max choosers.'
      end
    end
    topic.category = params[:topic][:category]
    topic.save
    redirect_to_sign_up(params[:id])
  else
    set_values_for_new_topic
    if @assignment.is_microtask?
      @sign_up_topic.micropayment = params[:topic][:micropayment]
    end
    if @assignment.staggered_deadline?
      topic_set = Array.new
      topic = @sign_up_topic.id
    end
    if @sign_up_topic.save
      undo_link("Topic: \"#{@sign_up_topic.topic_name}\" has been created successfully. ")
      redirect_to edit_assignment_path(@sign_up_topic.assignment_id) + "#tabs-5"
    else
      render :action => 'new', :id => params[:id]
    end
  end
end

After Refactoring
Method: update_topic_info, check_after_create, create

def update_topic_info(topic)
  topic.topic_identifier = params[:topic][:topic_identifier]
  if SignedUpUser.find_with_topic_id(topic.id).nil? || topic.max_choosers == params[:topic][:max_choosers]
    topic.max_choosers = params[:topic][:max_choosers]
  else
    if topic.max_choosers.to_i < params[:topic][:max_choosers].to_i
      topic.update_waitlisted_users(params[:topic][:max_choosers])
      topic.max_choosers = params[:topic][:max_choosers]
    else
      flash[:error] = 'Value of maximum choosers can only be increased! No change has been made to max choosers.'
    end
  end
end

The method update_topic_info updates topics' some attributes according to received parameters, such as permitted max number of choosers and waitlisted users. This method is separated from function create because it doesn't really do any creation works.

def check_after_create
  if @assignment.is_microtask?
    @sign_up_topic.micropayment = params[:topic][:micropayment]
  end
  if @assignment.staggered_deadline?
    topic_set = Array.new
    topic = @sign_up_topic.id
  end
end

The method check_after_create check if the assignment is a microtask and if it applies to staggered deadline. If these two conditions are met, then execute corresponding actions. These steps should be separated from create function because it doesn't actually do works related with creation.

def create
  topic = SignUpTopic.find_with_name_and_assignment_id(params[:topic][:topic_name], params[:id]).first
  if topic
    update_topic_info(topic)
    topic.category = params[:topic][:category]
    topic.save
    redirect_to_sign_up(params[:id])
  else
    set_values_for_new_topic
    check_after_create
    if @sign_up_topic.save
      undo_link("Topic: \"#{@sign_up_topic.topic_name}\" has been created successfully. ")
      redirect_to edit_assignment_path(@sign_up_topic.assignment_id) + "#tabs-5"
    else
      render action: 'new', id: params[:id]
    end
  end
end

In create method, the two separated methods were called. By this way, function create will be much shorter and some works do not belong to it can be finished by other methods.

Deleting redundant button

When we move the mouse on the Assignment, there is a "Edit signup sheet" button in the popup menu. It is a redundant button since there already has such a function in Assignment "Edit" menu. What's more it's more reasonable to put this function in "Edit" Assignment because signup sheet is a part of the assignment. So we removed this button. Sometimes after making some changes, the display topics function didn’t work due to some reasons. We’ve fixed those bugs and make sure it still works after our last change.
We delete the following lines in app/views/tree_display/actions/_assignments_actions.html.erb

<% if signup_topic %>
  ...
<% end %>

The origin popup menu is as following picture, there is a edit signup sheet button:


The new popup menu is:


After modifying, the other assignment functions all works well.

Renaming controller

According to the controller name convention<ref>RubyOnRails-Renaming Controller</ref>, a controller should be a noun. However, here SignUp in camel case is a verb. We should modify it to a noun written as Signup. As a result, we rename all class name SignUpSheetController to SignupSheetController. Then the controller name sign_up_sheet_controller is revised into signup_sheet_controller automatically.
Here we also make sure every reference for this controller has been changed to match the new controller name. For example, in views/signup_sheet/_action.html.erb file, we must make sure this render path is correct as follows:

 <%= render :partial => '/signup_sheet/all_actions', :locals => {:i=>i, :topic=>topic} %> 

Following global rules

According to Global Rules, there are some codes that need some improvements to make it follow the rules correctly. Here are some of what we have done:

• change users_team.size == 0 to users_team.zero?

change:
  if users_team.size == 0
to:
  if users_team.zero?
or:
  if users_team.empty?

• Use key: 'value', not :key => 'value'

change:
  redirect_to :action => 'add_signup_topics_staggered', :id => assignment_id
to:
  redirect_to action: 'add_signup_topics_staggered', id: assignment_id

• Use if (var) instead of if (var == true)

change:
  (assignment.staggered_deadline == true)?
to:
  (assignment.staggered_deadline)?

• Use good conditional statements

change:
  if user_signup_topic.is_waitlisted == false
to:
  unless user_signup_topic.is_waitlisted
 

References

<references/>