CSC/ECE 517 Spring 2015/oss E1509 lds: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
Line 39: Line 39:
=== RESTful style implementation ===
=== RESTful style implementation ===
----
----
The purpose of the <b>list</b> method in the <b>StudentQuizzes controller</b> is to list all the quizzes that are available to a particular user for a particular assignment. RESTful guidelines state that a method that returns a list of all available objects, in this case the quizzes, should be named as <b>index</b>. 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 student_quizzes_controller
The purpose of the <b>list</b> method in the <b>SignupSheet controller</b> 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 <b>index</b>. 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.
 
<b>Before Refactoring :</b>
<br> Method : list
<pre>
def list
\t@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])
        #end
 
        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
 
          #Find whether the user has signed up for any topics; if so the user won't be able to
          #sign up again unless the former was a waitlisted topic
          #if team assignment, then team id needs to be passed as parameter else the user's id
          users_team = SignedUpUser.find_team_users(params[:id],(session[:user].id))
 
          if users_team.size == 0
            @selected_topics = nil
          else
            #TODO: fix this; cant use 0
            @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
</pre>


=== Moving inappropriate functionality from controller to model ===
=== Moving inappropriate functionality from controller to model ===

Revision as of 19:38, 21 March 2015

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

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
\t@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])
        #end

        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

          #Find whether the user has signed up for any topics; if so the user won't be able to
          #sign up again unless the former was a waitlisted topic
          #if team assignment, then team id needs to be passed as parameter else the user's id
          users_team = SignedUpUser.find_team_users(params[:id],(session[:user].id))

          if users_team.size == 0
            @selected_topics = nil
          else
            #TODO: fix this; cant use 0
            @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

Moving inappropriate functionality from controller to model

Breaking down long methods

Deleting redundant button

Renaming controller

Following global rules

References

<references/>