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.

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>https://github.com/expertiza/expertiza</ref>. 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 .

Problem Statement

Classes involved

* sign_up_sheet.rb 
* sign_up_topic.rb 
* assignments_controller.rb
* response.rb
* and possibly other model classes

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.

   a. def confirm_topic(creator_id, topic_id, assignment_id)

   b. def delete_signup

   c. def delete_signup_for_topic(assignment_id,topic_id)

   d. def other_confirmed_topic_for_user(assignment_id, creator_id)

   e. def signup

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

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>http://guides.rubyonrails.org/routing.html#the-purpose-of-the-rails-router</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

RESTful style implementation

---

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

Before Refactoring:
Method: list

def list
  @assignment_id = params[:id]
  @sign_up_topics = SignUpTopic.where( ['assignment_id = ?', params[:id]]).all
  @slots_filled = SignUpTopic.find_slots_filled(params[:id])
  @slots_waitlisted = SignUpTopic.find_slots_waitlisted(params[:id])
  @show_actions = true
  @priority = 0
  assignment=Assignment.find(params[:id])

  if assignment.due_dates.find_by_deadline_type_id(1)!= nil
    unless !(assignment.staggered_deadline? and assignment.due_dates.find_by_deadline_type_id(1).due_at < Time.now )
      @show_actions = false
    end
    users_team = SignedUpUser.find_team_users(params[:id],(session[:user].id))
    if users_team.size == 0
      @selected_topics = nil
    else
      @selected_topics = SignUpSheetController.other_confirmed_topic_for_user(params[:id], users_team[0].t_id)
    end
    SignUpTopic.remove_team(users_team, @assignment_id)
  end
end

After Refactoring:
Method: index

def index
  @assignment_id = params[:id]
  @sign_up_topics =SignUpTopic.find_with_assignment_id( params[:id]).all
  filled_and_waitlisted_topics(@assignment_id)
  @show_actions = true
  @priority = 0
  assignment=Assignment.find(params[:id])
  if assignment.due_dates.find_by_deadline_type_id(1)!= nil
    unless !(assignment.staggered_deadline? and assignment.due_dates.find_by_deadline_type_id(1).due_at < Time.now )
      @show_actions = false
    end
    users_team = SignedUpUser.find_team_users(params[:id],(session[:user].id))
    if users_team.size == 0
      @selected_topics = nil
    else
      @selected_topics = SignupSheetController.other_confirmed_topic_for_user(params[:id], users_team[0].t_id)
    end
    SignUpTopic.remove_team(users_team, @assignment_id)
  end
end

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 role and different usage 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 remove 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:

def create
 topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id:  params[:id]).first
 #if the topic already exists then update
 if topic
   update_topic_info(topic)

After Refactoring:
In signup_sheet_controller.rb:

def create
 topic = SignUpTopic.find_with_name_and_assignment_id(params[:topic][:topic_name], params[:id]).first
 #if the topic already exists then update
 if topic
   update_topic_info(topic)

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:

def set_duedate_info
 @review_rounds = Assignment.find(params[:id]).get_review_rounds
 @topics = SignUpTopic.where(assignment_id: params[:id])
 #Use this until you figure out how to initialize this array
 @duedates = SignUpTopic.find_by_sql("SELECT s.id as topic_id FROM sign_up_topics s WHERE s.assignment_id = " + params[:id].to_s)
 unless @topics.nil?

After Refactoring:
In signup_sheet_controller.rb:

def set_duedate_info
 @review_rounds = Assignment.find(params[:id]).get_review_rounds
 @topics = SignUpTopic.find_with_assignment_id( params[:id])
 #Use this until you figure out how to initialize this array
 @duedates = SignUpTopic.find_topic_id_with_assignment_id(params[:id].to_s)
 unless @topics.nil?

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 %>
  <% if @assignment.staggered_deadline == true %>
    <%= link_to image_tag('/assets/tree_view/edit-signup-sheet-24.png', :title => 'Edit signup sheet'), {:controller => 'sign_up_sheet', :action => 
            'add_signup_topics_staggered', :id => node.node_object_id}, {:title => 'Edit signup sheet'} %>
  <% else %>
    <%= link_to image_tag('/assets/tree_view/edit-signup-sheet-24.png', :title => 'Edit signup sheet'), {:controller => 'sign_up_sheet', :action => 
            'add_signup_topics', :id => node.node_object_id}, {:title => 'Edit signup sheet'} %>
  <% end %>
  <% if @assignment.is_intelligent %>
    <%= link_to image_tag('/assets/tree_view/run-lottery.png', :title => 'Assign teams intelligently', :onClick => "showIntelligentAssignmentDialog()"), 
            :controller => 'lottery', :action => 'run_intelligent_bid', :id => @assignment.id %>
  <% end %>
  <script type="text/javascript">
    function showIntelligentAssignmentDialog() {
        jQuery( "#intelligent_assignment_dialog" ).dialog({ closeText: "hide", modal: true, resizable: false, width: 500 });
    }
  </script>
  <div id="intelligent_assignment_dialog" title="Running intelligent assignment" style="display: none;">
    <p>Intelligent Assignment is running. Please wait for the action to be completed.</p>
  </div>
<% else %>
  <% if @assignment.staggered_deadline == true %>
    <%= link_to image_tag('/assets/tree_view/add-signup-sheet-24.png', :title => 'Add signup sheet'), {:controller => 'sign_up_sheet', :action => 
            'add_signup_topics_staggered', :id => node.node_object_id}, {:title => 'Add signup sheet'} %>
  <% else %>
    <%= link_to image_tag('/assets/tree_view/add-signup-sheet-24.png', :title => 'Add signup sheet'), {:controller => 'sign_up_sheet', :action => 
            'add_signup_topics', :id => node.node_object_id}, {:title => 'Add signup sheet'} %>
  <% end %>
<% end %>

Renaming controller

According to the Controller name convention<ref>http://guides.rubyonrails.org/action_controller_overview.html</ref>, a Controller should be a singular 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 namesign_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)?

References

<references/>