CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb

Refactor sign up sheet controller.rb

This page gives a description of the refactoring done in the sign_up_sheet_controller.rb as part of the OSS project.

About Expertiza

---

Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

Description of the project

---

The following is an Expertiza based OSS project which deals with the signup_sheet_controller.rb. It focuses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing some redundant code. The goal of this project is to attempt to make this part of the application easier to read and maintain.

About Signup Sheet Controller

---

This controller is used while signing up for a topic from the signup sheet. This controller allows users to signup as an instructor as well as a student. An instructor has the authority to do CRUD operations on topics and enrolled/waitlisted members of each topic. The instructor can also add or update deadlines for the assignment.

Issues and Solutions

---

• Issue 1 - Inconsistent naming. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf.“sign_up_as_instructor”.
  • Solution- Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.

• Issue 2 - Create method has and if-else condition determining if create or update should be called. Create method should not be responsible for calling update.
  • Solution-Created another method 'call_create_or_update' which has the if-else condition that determines if create or update is called.

• Issue 3 - Update method has a plethora of instance variables defined before updating. These might not be necessary. Decide whether so many instance variables are really needed.
  • Solution-Used a single update_attributes method in place of the many instance variables.

• Issue 4 - Destroy has a misleading else flash message.
  • Solution-Changed the misleading flash message in the else condition to be more straightforward.

• Issue 5 - Add_signup_topics_staggered does not do anything. Find out why add_signup_topics_staggered simply calls add_signup_topics, and why separate functions are needed.
  • • Solution-It was found that the method add_signup_topics_staggered was unnecessary and was removed.

• Issue 6 - Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
  • Solution-Several method names were appropriately improved and renamed.

• Issue 7 - What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed.
  • Solution-It was found that both the signup_as_instructor and signup_as_instructor_action methods are needed and their use was explained with proper comments.

• Issue 8 - The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
  • Solution-Provided comments for blocks of code and created helper methods.

• Issue 9 - Refactor participants variable in load_add_signup_topics
  • Solution-Refactored the participants variable and improved readability.

• Issue 10 - Signup_as_instructor_action has a complicated if-else ladder.
  • Solution-The if-else ladder was made more elegant and readable.

• Issue 11 - Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle.
  • Solution-The excess code in both the delete_signup and delete_signup_as_instructor methods was removed and replaced by just one 'get_status' method call.

Code Improvements

---

• The create method was responsible for calling the update method directly. Generally the create method should not directly call the update method.

Before

def create
    topic = SignUpTopic.where(topic_name: params[:topic][:topic_name], assignment_id: params[:id]).first
    if topic.nil?
      setup_new_topic
    else
      update_existing_topic topic
    end
  end

After

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

# This method appropriately calls the create topic or update existing topic method
  def call_create_or_update(topic)
    if topic.nil?
      setup_new_topic
    else
      update_existing_topic topic
    end
  end

• The update method has a lot of instance variables initialized. The method has been changed in the following ways

Before

def update
    @topic = SignUpTopic.find(params[:id])
    if @topic
      @topic.topic_identifier = params[:topic][:topic_identifier]
      update_max_choosers @topic
      @topic.category = params[:topic][:category]
      @topic.topic_name = params[:topic][:topic_name]
      @topic.micropayment = params[:topic][:micropayment]
      @topic.description = params[:topic][:description]
      @topic.link = params[:topic][:link]
      @topic.save
      undo_link("The topic: "#{@topic.topic_name}" has been successfully updated. ")
    else
      flash[:error] = "The topic could not be updated."
    end
  end

After

def update
    @topic = SignUpTopic.find(params[:id])
    if @topic
    update_max_choosers @topic
      updated_max_choosers = @topic.max_choosers
      @topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
      undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
    else
      flash[:error] = "The topic could not be updated."
    end
  end

• Fixed the if-else ladder in signup_as_instructor_action action:

Before

  def signup_as_instructor_action
    user = User.find_by(name: params[:username])
    if user.nil? # validate invalid user
      flash[:error] = "That student does not exist!"
    else
      if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
        if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
          flash[:success] = "You have successfully signed up the student for the topic!"
          ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
        else
          flash[:error] = "The student has already signed up for a topic!"
          ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
        end
     else
        flash[:error] = "The student is not registered for the assignment!"
        ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
      end
    end
    redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
  end

After

 def signup_as_instructor_action
    user = User.find_by(name: params[:username])
    if user.nil? # validate invalid user
      flash[:error] = "That student does not exist!"
    else
     if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id] and SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
        flash[:success] = "You have successfully signed up the student for the topic!"
        ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
      elsif AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
        flash[:error] = "The student has already signed up for a topic!"
        ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
      else
        flash[:error] = "The student is not registered for the assignment!"
        ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
      end
    end
    redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
  end

• The delete_signup method has repeated flash messages. These messages are moved to a new method to follow the DRY principle.

Before

  def delete_signup
    participant = AssignmentParticipant.find(params[:id])
    assignment = participant.assignment
    drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
    # A student who has already submitted work should not be allowed to drop his/her topic!
    # (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
    # If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
    # If there is a drop topic deadline, student cannot drop topic after this deadline.
    if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
      flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
    elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
      flash[:error] = "You cannot drop your topic after the drop topic deadline!"
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
    else
      delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
      flash[:success] = "You have successfully dropped your topic!"
      ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
    end
    redirect_to action: 'list', id: params[:id]
  end

After

 def delete_signup
    participant = AssignmentParticipant.find(params[:id])
    assignment = participant.assignment
    drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
    # A student who has already submitted work should not be allowed to drop his/her topic!
    # (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
    # If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
    # If there is a drop topic deadline, student cannot drop topic after this deadline.
   flash_message = get_status("student", participant, assignment, drop_topic_deadline)
    if flash_message[:type] == "error"
      flash[:error] = flash_message[:message]
    else
   end
    redirect_to action: 'list', id: params[:id]
  end

def get_status(status_for, participant, assignment, drop_topic_deadline)
    if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
      flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
      if status_for == "student"
        return {:type => "error", :message => "You have already submitted your work, so you are not allowed to drop your topic."}
      end
      if status_for == "instructor"
        return {:type => "error", :message => "The student has already submitted their work, so you are not allowed to remove them."}
      end
    elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
      flash[:error] = "You cannot drop a student after the drop topic deadline!"
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
      if status_for == "student"
        return {:type => "error", :message => "You cannot drop your topic after the drop topic deadline!"}
      end
      if status_for == "instructor"
        return {:type => "error", :message => "You cannot drop a student after the drop topic deadline!"}
      end
    else
      delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
      flash[:success] = "You have successfully dropped the student from the topic!"
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
      if status_for == "student"
        return {:type => "success", :message => "You have successfully dropped your topic!"}
      end
      if status_for == "instructor"
        return {:type => "success", :message => "You have successfully dropped the student from the topic!"}
      end
    end
    redirect_to controller: 'assignments', action: 'edit', id: assignment.id
  end

• The destroy method had a misleading else flash message.

Before

def destroy
    @topic = SignUpTopic.find(params[:id])
    assignment = Assignment.find(params[:assignment_id])
    if @topic
      @topic.destroy
      undo_link("The topic: \"#{@topic.topic_name}\" has been successfully deleted. ")
    else
      flash[:error] = "The topic could not be deleted."
    end

After

def destroy
    @topic = SignUpTopic.find(params[:id])
    assignment = Assignment.find(params[:assignment_id])
    if @topic
      @topic.destroy
      undo_link("The topic: \"#{@topic.topic_name}\" has been successfully deleted. ")
    else
      flash[:error] = "The topic to be deleted does not exist. "
    end

• The destroy test with 'when topic cannot be found' context in signup_sheet_controller_spec.rb was modified to accomodate the new change.

    expect(flash[:error]).to eq('The topic to be deleted does not exist. ')

Testing

---

As this was a Refactoring project, all the features will work like they were previously working and hence there is no live demo(deployment) required.

There are no new tests written to test the changes. The testing was done by using the existing tests in the spec/controllers/signup_sheet_controller_spec.rb

Automated Testing with RSpec

Link to YouTube video showing the RSpec testing.

---

Testing with Travis CI

Results/Conclusions

---

After refactoring the code, following is achieved:

1. Made code more readable e.g. fixed the if-else ladder in Issue 10.
2. Cleaned up the code and made it tidier.
3. Removed redundant, unused code(like the add_signup_topics_staggered method) and comments.
4. Made some things more generic (like get_status method)
5. Kept the code follow the DRY principle (delete_signup and delete_signup_as_instructor now satisfy the DRY principle)
6. Combined and disposed "Like" or "Similar" code.
7. Splitted out long functions into more manageable bite.
8. Created reusable code
9. Made the class and function cohesion better.

Project Mentors

1. Edward Gehringer (efg@ncsu.edu)
2. Ramya Vijayakumar (rvijaya4@ncsu.edu)

Team Members

1. Rajit Bharambe (rbharam@ncsu.edu)
2. Saahil Chawande (schawan@ncsu.edu)
3. Sravan Matta (skmatta@ncsu.edu)

References

1. Expertiza on GitHub
2. GitHub Project Repository Fork
3. The live Expertiza website
4. RSpec Testing YouTube video
5. Rspec Documentation
6. Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin