CSC/ECE 517 Fall 2014/oss E1453 syy

E1453. Refactoring AssignmentsController

This page provides a description of the Expertiza based OSS project. This project aimed at refactoring the AssignmentsController as well as adding some missing functionality related to topics within an assignment.

Introduction to Expertiza

Expertiza is a project developed using Ruby on Rails platform. It provides features like peer review, team assignments and submission of projects. This can be achieved by submitting code base, URL of hosted code on remote server and Wiki submissions. It is an open source application and the code can be cloned from github. This application provides an efficient way to manage assignments, grades and reviews. This makes the process easier and faster when the class strength is large.

Expertiza is supported by National Science Foundation under Grant No. 0536558. Additional funding from the NCSU Learning in a Technology-Rich Environment (LITRE) program, the NCSU Faculty Center for Teaching and Learning, the NCSU STEM Initiative, and the Center for Advanced Computing and Communication.

Project Resources

GitHub Project Repository

VCL IP Address

Expertiza Wiki

Problem Statement

Currently the topics tab is not usable to the instructor when he is editing a current assignment or creating a new one. This functionality is necessary as it enables the instructor to create/delete/edit/view topics present under the selected assignment. Currently, the instructor achieves this using the pop-up menu that appears adjacent every assignment on the page that lists all the assignments. We were required to implement the topics tab.

The functionality to display the topics under an assignment is duplicated between the AssignmentController views and the SignupSheetController views. Both the instructor and the student can view the topics by selecting an assignment and we were required to remove this duplication in the views.

Re-factored Code Cases

Case 1 : Refactoring the AssignmentsController and SignupSheetController to use same views

Current Scenario

The AssignmentsController displayed topics to the instructor while he was editing an assignment. It used _add_signup_topics.html.erb partial to render topics under the topics tab. The SignupSheetController displayed topics to a student who wanted to view signup topics under an assignment. It was found that it had its own copy namely _add_signup_topcis.html.erb to display topics to the student. We wanted to have these two controllers use the same views to render the same thing which was list of topics under an assignment. We made the AssignmentsController reuse the _add_signup_topcis.html.erb partial under the SignupSheetController.

view:assignments/edit.html.erb
This view contains all the tabs to display like general,rubrics,review_strategy, topics which are related to the selected assignment. tabs-5 is the topics tab which we are interested in.

   <div id="tabs-3">
        <%= render 'assignments/edit/review_strategy' %>
   </div>
   <div id="tabs-4">
        <%= render 'assignments/edit/due_dates' %>
   </div>
   <div id="tabs-5">
        <%= render 'assignments/edit/add_signup_topics' %>
   </div>

   <div id="tabs-3">
        <%= render 'assignments/edit/review_strategy' %>
   </div>
   <div id="tabs-4">
        <%= render 'assignments/edit/due_dates' %>
   </div>
   <div id="tabs-5">
        <%= render '/sign_up_sheet/add_signup_topcis.html' %>
   </div>

method:add_signup_topics
It is similar to the above function except that all the topics and review/submission rounds have the similar deadlines.

 def add_signup_topics
   load_add_signup_topics(params[:id])
 end

Conditional call
We had to make conditional check every time the function is called for the assignment type. For instance, below method redirects to the /add_signup_topics or the /add_signup_topics_staggered page depending on assignment type

def redirect_to_sign_up(assignment_id)
   assignment = Assignment.find(assignment_id)
   if assignment.staggered_deadline == true
     redirect_to :action => 'add_signup_topics_staggered', :id => assignment_id
   else
     redirect_to :action => 'add_signup_topics', :id => assignment_id
   end
end

After Changes

We have redundant code in two methods so we merged add_signup_topics_staggered and add_signup_topics into add_signup_topics. The action calls single method for both type of assignments and renders views based on condition of staggered or non-staggered assignment.

def add_signup_topics
   load_add_signup_topics(params[:id])
   if @assignment.staggered_deadline ==true
      ....
      
      #below functionality finds allows to set due dates for topics of assignment
      @duedates = SignUpTopic.find_by_sql("SELECT s.id as topic_id FROM sign_up_topics s WHERE s.assignment_id = " + params[:id].to_s)
      ....
      
      #Render a view different for staggered deadlines assignment from the add_sign_up.html.erb
      render "add_signup_topics_staggered"
  end
end

Single method call
At several places in the project, there was separate method call based on staggered and non-staggered assignments. We have made a single method call and condition check in various files ,for type of assignment, has been removed as part of re-factoring.

def redirect_to_sign_up(assignment_id)
   assignment = Assignment.find(assignment_id)
   redirect_to :action => 'add_signup_topics', :id => assignment_id
end

Similarly the method call is re-factored in various views. For instance, for view _assignments_actions.html.erb

Before Changes

<% if @assignment.staggered_deadline == true %>
   <%= link_to image_tag('/images/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('/images/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 %>

After Changes

 <%= link_to image_tag('/images/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'} %>

Case 2 : Method Extraction

Improved readability by extracting two separate functionalities from the same method into separate methods with appropriate naming convention and responsibilities.

Current Scenario

The add_signup_topics calculated due dates for separate topics for a Staggered assignment with individual Topic deadlines.

def add_signup_topics
  load_add_signup_topics(params[:id])
  if @assignment.staggered_deadline ==true
     @review_rounds = Assignment.find(params[:id]).get_review_rounds
     @topics = SignUpTopic.find_all_by_assignment_id(params[:id])
     
    #below functionality finds allows to set due dates for topics of assignment
     @duedates = SignUpTopic.find_by_sql("SELECT s.id as topic_id FROM sign_up_topics s WHERE s.assignment_id = " + params[:id].to_s)
     if !@topics.nil?
       i=0
       @topics.each { |topic|
       ......
       i = i + 1
     end
     
     #Render a view different for staggered deadlines assignment from the add_sign_up.html.erb
     render "add_signup_topics_staggered"
 end
end

After Changes

The logic to calculate due_dates was moved to a separate method called assignment_due_date , as this is only valid for staggered assignments and hence it should be placed in a different method instead of merging it the add_signup_topics

def add_signup_topics
   load_add_signup_topics(params[:id])
 if @assignment.staggered_deadline ==true
     @review_rounds = Assignment.find(params[:id]).get_review_rounds
     @topics = SignUpTopic.find_all_by_assignment_id(params[:id])
    
  #Function call to set up due dates
     assignment_due_date
     
  #Render a view different from the add_sign_up.html.erb  , so rendering a view for staggered deadlines.
     render "add_signup_topics_staggered"
   end
 end

 def assignment_due_date
    #below functionality finds allows to set due dates for topics of assignment
     @duedates = SignUpTopic.find_by_sql("SELECT s.id as topic_id FROM sign_up_topics s WHERE s.assignment_id = " + params[:id].to_s)
     if !@topics.nil?
       i=0
       @topics.each { |topic|
       ......
       i = i + 1
     end
 end

Case 3 : Modularize Code in application.rb

Current Scenario

In the assignment.rb model, there exists one method(check_condition) which checks whether review, meta-review, etc are allowed or not. This method makes database calls to TopicDeadline table, DueDate and DeadlineType tables. Following is the code which was existing before the changes were made. It has the logic to find the nearest due date that hasn't passed.

assignment.rb

    drop_topic_deadline_id = DeadlineType.find_by_name('drop_topic').id
   if self.staggered_deadline?
     
     if topic_id
       
       next_due_date = TopicDeadline.find(:first, :conditions => ['topic_id = ? and due_at >= ? and deadline_type_id <> ?', 
                                  topic_id, Time.now, drop_topic_deadline_id], :order => 'due_at')
     else
       
       next_due_date = TopicDeadline.find(:first, :conditions => ['assignment_id = ? and due_at >= ? and deadline_type_id <> ?', 
                                 self.id, Time.now, drop_topic_deadline_id], :joins => {:topic => :assignment}, :order => 'due_at')
     end
   else
     next_due_date = DueDate.find(:first, :conditions => ['assignment_id = ? and due_at >= ? and deadline_type_id <> ?', 
                                 self.id, Time.now, drop_topic_deadline_id], :order => 'due_at')
   end

After changes

The database queries have been moved to their respective models. The queries to TopicDeadline table, DueDate table and DeadlineType table have been moved to their models. Below is the new code in assignments.rb model which calls TopicDeadline.rb, DueDate.rb and DeadlineType.rb respectively for the queries in their tables:

assignment.rb model

if self.staggered_deadline?
  next_due_date = TopicDeadline.find_next_due_date(topic_id,self.id)
else
  next_due_date = DueDate.find_next_due_date(self.id)
end

New find_next_due_date defined in topic_deadline.rb

def self.find_next_due_date(topic_id,assignment_id)
   drop_topic_deadline_id = DeadlineType.find_drop_topic_deadline_id()
   if topic_id
     TopicDeadline.find(:first, :conditions => ['topic_id = ? and due_at >= ? and deadline_type_id <> ?', 
                        topic_id, Time.now, drop_topic_deadline_id], :order => 'due_at')
   else
     TopicDeadline.find(:first, :conditions => ['assignment_id = ? and due_at >= ? and deadline_type_id <> ?', 
                        assignment_id, Time.now, drop_topic_deadline_id], :joins => {:topic => :assignment}, :order => 'due_at')
   end
end

New find_next_due_date method defined in due_date.rb

def self.find_next_due_date(assignment_id
  drop_topic_deadline_id = DeadlineType.find_drop_topic_deadline_id()
  DueDate.find(:first, :conditions => ['assignment_id = ? and due_at >= ? and deadline_type_id <> ?', 
               assignment_id, Time.now, drop_topic_deadline_id], :order => 'due_at')
end

New find_drop_topic_deadline_id method defined in deadline_type.rb

def self.find_drop_topic_deadline_id()
  DeadlineType.find_by_name('drop_topic').id
end

Case 4 : DeadCode Elimination in sign_up_sheet_controller.rb

The local variables topic_set and topic under the staggered deadline check statement were never used in the entire scope of the project , it introduced code smells. So it was checked if there was any reference to this code, and upon discovering it was not used anywhere, we decided to eliminate this deadcode.

Current Scenario

def create_default_for_microtask
   .....
 
   if @assignment.staggered_deadline?
     topic_set = Array.new
     topic = @sign_up_topic.id
   end
   
   .....
 end

After Changes

def create_default_for_microtask
   .....
 
   .....
 end

Steps to verify changes

Log into the application with the user having instructor's role or super user's role.

1. Go to Manage -> Assignments and hover over the assignment “StaggeredDeadlines_Refactoring”.

2. Click on “New Topic”.

3. Enter the details of the topic you want to create. Create at least 2 topics for better analysis of the project.

4. Now once you have created two new topics , then Click on “Show start/due date”.

5. You should be able to view the new start and due dates of your topics confirming these are the staggered deadlines for “StaggeredDeadlines_Refactoring” which has many topics with different deadlines.

6. Now to view the difference between the Stagerred and Non-Staggered Deadlines, follow steps.

7. Go to Manage -> Assignments and hover over “SALT Demo” and click on “Edit Signup sheet”.

8. Click on “New Topic” and fill in the details for a new topic.

9. Now you will be able to see that there is no option for “Show Start/Due Date” which is because of the reason that this is not a staggered assignment.

10. Hence there is only one due date and has no dependencies on others.

Concise Report on Refactoring

The project had redundant code, multiple functionalities in a method and extra conditional statements in controller, model and view files. We analyzed each occurrence of these issues, thus creating a list which needed to be refactored. Controllers like sign_up_sheet_controler, assignment.rb, due_date.rb,deadline_types.rb along with other files were modified. It's very difficult to explain the sequential and detailed procedure, but it has been explained in the sections above within the capacity of the Wiki.

Additional Learning and Future Work

In current implementation, our database has two tables for staggered and non-staggered deadlines. For any future improvements, we can make use of STI (Single Table Inheritance) and analyze if applying STI would really impact query performance when application runs and extracts data from database.

References

Expertiza
Single Table Inheritance
Code Refactoring
Code smell