CSC/ECE 517 Fall 2017/E1752 Refactor assignments controller

Introduction

Expertiza is an open source webapp built on Ruby on Rails stack. It provides a platform to students with various features like peer-reviewing projects, submitting work, form teams, viewing grades etc. The project is being built and maintained by students and faculty at NCSU.

About Assignments Controller

The file assignments_controller.rb handles the logic behind Expertiza assignments and enables the creation and managements of assignments by instructional staff. Besides regular CRUD operations, it also integrates multiple parts of the expertiza components, such as users of different authorizations and priorities, corresponding courses associated with the assignment, due date and so on. Therefore, further unit and integration tests are required to guarantee that these operations work as expected. And also refactoring on the code in this controller is needed to make it more readable and reduce the complexity at the same time. Our tasks are based on the motivations above.

About Software Testing

Software testing is a process of examining a program or application with the intent of finding the software bugs. In software developemnt, it is usually defined as the process of validating and verifying that a software program or application or product meets the specified business and technical requirements.

Additionaly, software is tested to:

• Detect and rectify and errors made during the development phase,
• Ensure customer satisfaction in the application,
• Ensure the quality of the software product and
• Optimize the performance of the system.

Our project is guided by the Test-First Development principle. We write passing and failing tests for use cases of the assignment controller, and then turn to refactor the code. During the project, we implement the tests and refactor in Agile methodology with our mentor Zhewei through weekly delivery.

RSpec Testing

RSpec is a 'Domain Specific Language' (DSL) testing tool written in Ruby to test Ruby code. It is a behavior-driven development (BDD) framework which is extensively used in the production applications. The basic idea behind this concept is that of Test Driven Development(TDD) where the tests are written first and the development is based on writing just enough code that will fulfill those tests followed by refactoring.

The Expertiza project makes extensive use of RSpec to test the various components of the application. Tests of our tasks are in the code file: spec/controllers/assignments_controller_spec.rb

Testing actions perfomed

• Creating new tests for each method,
• Testing the each method and making sure the tests pass.

Testing results

All 22 tests were passed with zero failure on the final version of project code.

About Refactoring

Refactoring of computer software or code is the process of restructuring existing software code without changing its external behavior or performance of the software application. Refactoring improves nonfunctional attributes of the software. Refactoring aids in code readability and keeps the code well maintained and easy to understand.

Refactoring actions perfomed

• Replace magic numbers in deadline_type_id by constants with reasonable names;
• Formatting the code for readability and convention;
• Split large chunks of code in to smaller manageable chunks;
• Extract simpler and specific methods out of blocks of codes to remove code duplications using DRY principle;
• Rename existing methods with meaningful names for better readability without change on external functionality.

Refactor edit method

Actions: code block split, method extraction and add meaningful constants

Original

  def edit
    # give an error message is instructor have not set the time zone.
    if current_user.timezonepref.nil?
      flash.now[:error] = "You have not specified your preferred timezone yet. Please do this before you set up the deadlines."
    end
    @topics = SignUpTopic.where(assignment_id: params[:id])
    @assignment_form = AssignmentForm.create_form_object(params[:id])
    @user = current_user

    @assignment_questionnaires = AssignmentQuestionnaire.where(assignment_id: params[:id])
    @due_date_all = AssignmentDueDate.where(parent_id: params[:id])
    @reviewvarycheck = false
    @due_date_nameurl_notempty = false
    @due_date_nameurl_notempty_checkbox = false
    @metareview_allowed = false
    @metareview_allowed_checkbox = false
    @signup_allowed = false
    @signup_allowed_checkbox = false
    @drop_topic_allowed = false
    @drop_topic_allowed_checkbox = false
    @team_formation_allowed = false
    @team_formation_allowed_checkbox = false
    @participants_count = @assignment_form.assignment.participants.size
    @teams_count = @assignment_form.assignment.teams.size

    if @assignment_form.assignment.staggered_deadline == true
      @review_rounds = @assignment_form.assignment.num_review_rounds
      @assignment_submission_due_dates = @due_date_all.select {|due_date| due_date.deadline_type_id == 1 }
      @assignment_review_due_dates = @due_date_all.select {|due_date| due_date.deadline_type_id == 2 }
    end

    # Check if name and url in database is empty before webpage displays
    @due_date_all.each do |dd|
      @due_date_nameurl_notempty = is_due_date_nameurl_notempty(dd)
      @due_date_nameurl_notempty_checkbox = @due_date_nameurl_notempty
      @metareview_allowed = is_meta_review_allowed?(dd)
      @drop_topic_allowed = is_drop_topic_allowed?(dd)
      @signup_allowed = is_signup_allowed?(dd)
      @team_formation_allowed = is_team_formation_allowed?(dd)

      if dd.due_at.present?
        dd.due_at = dd.due_at.to_s.in_time_zone(current_user.timezonepref)
      end
      if  @due_date_nameurl_notempty && @due_date_nameurl_notempty_checkbox &&
          (@metareview_allowed || @drop_topic_allowed || @signup_allowed || @team_formation_allowed)
        break
      end
    end

    @assignment_questionnaires.each do |aq|
      unless aq.used_in_round.nil?
        @reviewvarycheck = 1
        break
      end
    end
    @due_date_all = update_nil_dd_deadline_name(@due_date_all)
    @due_date_all = update_nil_dd_description_url(@due_date_all)

    # only when instructor does not assign rubrics and in assignment edit page will show this error message.
    if !empty_rubrics_list.empty? && request.original_fullpath == "/assignments/#{@assignment_form.assignment.id}/edit"
      rubrics_needed = needed_rubrics(empty_rubrics_list)
      flash.now[:error] = "You did not specify all the necessary rubrics. You need " + rubrics_needed +
          " of assignment <b>#{@assignment_form.assignment.name}</b> before saving the assignment. You can assign rubrics <a id='go_to_tabs2' style='color: blue;'>here</a>."
    end

    if @assignment_form.assignment.directory_path.nil? || @assignment_form.assignment.directory_path.empty?
      flash.now[:error] = "You did not specify your submission directory."
    end
  end

After refactoring

  def edit
    handle_current_user_timezonepref_nil

    edit_params_setting

    assignment_form_assignment_staggered_deadline?

    @due_date_all.each do |dd|
      check_due_date_nameurl_notempty(dd)

      adjust_timezone_when_due_date_present(dd)

      break if validate_due_date
    end

    check_assignment_questionnaires_usage

    @due_date_all = update_nil_dd_deadline_name(@due_date_all)
    @due_date_all = update_nil_dd_description_url(@due_date_all)

    # only when instructor does not assign rubrics and in assignment edit page will show this error message.
    handle_rubrics_not_assigned_case

    handle_assignment_directory_path_nonexist_case
  end

Here we first grouped the blocks of code according to their similarity of functions. Then we extracted simpler methods from each code block to make it more readable and reduce the complexity and coupling. It is obvious that after refactoring, this part is simpler and easier to maintain. In addition, we also replaced the magic numbers by constants according to the deadline type.

Refactor update method

Actions: split long code block and extract specific methods

Original

  def update
    unless params.key?(:assignment_form)
      @assignment = Assignment.find(params[:id])
      @assignment.course_id = params[:course_id]
      if @assignment.save
        flash[:note] = 'The assignment was successfully saved.'
        redirect_to list_tree_display_index_path
      else
        flash[:error] = "Failed to save the assignment: #{@assignment.errors.full_messages.join(' ')}"
        redirect_to edit_assignment_path @assignment.id
      end
      return
    end

    @assignment_form = AssignmentForm.create_form_object(params[:id])
    @assignment_form.assignment.instructor ||= current_user
    params[:assignment_form][:assignment_questionnaire].reject! do |q|
      q[:questionnaire_id].empty?
    end

    if current_user.timezonepref.nil?
      parent_id = current_user.parent_id
      parent_timezone = User.find(parent_id).timezonepref
      flash[:error] = "We strongly suggest that instructors specify their preferred timezone to guarantee the correct display time. For now we assume you are in " + parent_timezone
      current_user.timezonepref = parent_timezone
    end
    if @assignment_form.update_attributes(assignment_form_params, current_user)
      flash[:note] = 'The assignment was successfully saved....'
    else
      flash[:error] = "Failed to save the assignment: #{@assignment_form.errors.get(:message)}"
    end
    redirect_to edit_assignment_path @assignment_form.assignment.id
  end

After refactoring

  def update
    unless params.key?(:assignment_form)
      assignment_form_key_nonexist_case_handler
      return
    end

    retrieve_assignment_form

    handle_current_user_timezonepref_nil

    feedback_assignment_form_attributes_update

    redirect_to edit_assignment_path @assignment_form.assignment.id
  end

The update method was refactored in the similar way to reduce the size of codes and improve readability. The definitions of those helper methods were placed at the bottom of the assignments_controller.rb.

Rename existing helper methods

Actions: rename for consistent Ruby code style

  is_due_date_nameurl_notempty → due_date_nameurl_notempty?
  is_meta_review_allowed? → meta_review_allowed?
  is_drop_topic_allowed? → drop_topic_allowed? 
  is_signup_allowed? → signup_allowed?
  is_team_formation_allowed? → team_formation_allowed?

After each refactoring procedure, we perform the testing to ensure the expected functionality in Rspec. For more cases, please check our GitHub project repo. The files we make change on are: app/controllers/assignments_controller.rb and app/helpers/deadline_helper.rb

Additional Links

Github project repo
Project Video Demo
Pull request

References

Rspec Documentation
Better Specs
Expertiza Wiki
Expertiza NCSU

Team

Shijie Li
Amey Deshmukh
Philip Musyoki