CSC/ECE 517 Spring 2022 - E2216: Refactor late policies controller

Background

The later policies controller file manages the SCRUM from late policies. Chiefly, the file checks if new or updated late policies are valid policies, in that they do not have the same name as an already existing policy, does not have a late penatly per unit time that surpasses the max late penalty, and does not have a late penalty surpassing 100.

Team Members

• Wyatt Plaga (wgplaga@ncsu.edu)
• Sujith Tumma (stumma2@ncsu.edu)
• Shawn Salekin (ssaleki@ncsu.edu)

Description about project

This wiki describes the work done to refactor late_policies_controller.rb. The main focus of the work went into removing code duplication, as well as simplifying the code for readability, for the create and update methods. This also includes adding code comments for understandibility.

Files Involved

File Modified:

• app/controllers/late_policies_controller.rb
• app/views/late_policies/new.html.erb
• app/views/late_policies/_form.html.erb
• spec/controllers/late_policies_controller_spec.rb
• db/schema.rb

File Added:

• db/migrate/20220321162332_add_private_to_late_policies.rb

Test Plan

This is a refactoring project, so performed manual tests to ensure the functionalities did not change. Here is how you can test it manually.

• Navigate over to the late policies index page: http://152.7.98.82:9090/late_policies
• Create a new policy. Try different input combinations, some valid or invalid. Keep an eye for the error messages
• Try updating the policy
• Try giving the same name to two different policies and see if you get proper error message
• Delete policies.
• Additionally, if you are able to log in as a different instructor, you should not see someone else's privately created

policies.

If all of these changes work as intended, we should be good.

Code Issues

1. All the late policies will show up on the index page, not just policies created by the current instructor. Need to create public & private late policies. Try to leverage code for private and public courses, assignments, and questionnaires.

2. create and update have some duplicate code. Fix that using a partial, _form.

3. Both create and update need comments and a better identifier naming.

4. Reduce code duplication. Find a way to make the code more DRY

5. Line 64: The if condition can be simplified.

6. Improve the variable name for same_policy_name. (Boolean variables can be named in a better way)

7. Add method comments

8. Find the use of instance variables in create and update method? Do we really need it? Can it be made local variables instead?

Code Changes

1. All the late policies will show up on the index page, not just policies created by the current instructor. Need to create public & private late policies. Try to leverage code for private and public courses, assignments, and questionnaires

Current code:

(No such code exists)

Modifications:

- add a column named 'private' on to the LatePolicy model
- In the controller#index method, filter the results by private != 0 or instructor_id = current_user_id

2. create and update have some duplicate code. Fix that using a partial, _form.

Filename: app/views/late_policies/new.html.erb

Current Code:

<%= form_for @late_policy, :url => { :action => "create" } do |f| %>
  <%= error_messages_for @late_policy %>

  <p>
  <label for="late_policy_name">Late policy name:</label><label style="color: #ff0000;">*</label><br/>
    <%= f.text_field 'policy_name' %>
  </p>
  <p>
  <label for="late_policy_penalty_points_per_unit">Penalty points per unit:</label><label style="color: #ff0000;">*</label><br/>
    <%= f.text_field 'penalty_per_unit' %>
  </p>
  <p>
  <label for="penalty_unit">Penalty unit:</label><label style="color: #ff0000;">*</label><br/>
    <!--select name=<='late_policy[:penalty_unit]'%>>
    <option value='Minute' selected>Minute</option>
    <option value='Hour' >Hour</option>
    <option value='Day' >Day</option>
    </select-->
    <% @units= %w[Minute Hour Day]%>
    <%= f.select "penalty_unit", @units %>
  </p>
  <p>
    <label for="late_policy_penalty_maximum">Maximum penalty:</label><label style="color: #ff0000;">*</label><br/>
    <%= f.text_field 'max_penalty' %>
  </p>

Modifications:

<%= form_for @penalty_policy, :url => { :action => "create" } do |f| %>
  <%= error_messages_for @penalty_policy %>
  <%= render partial: 'form', locals: { f: f } %>

3. Both create and update need comments and a better identifier naming.

Current identifiers names:

* invalid_penalty_per_unit (Problem with this is ,try to read "!invalid_penalty_per_unit". So, "valid_penalty" increases the readability. )
* policy_name_exists

Modifications:

Got rid of both identifier names and implemented functions that validate input and check the duplicity of late policy name
* valid_penalty
* duplicate_name_check

4. Reduce code duplication. Find a way to make the code more DRY

Extracted the validation done in both create and update into a seperate method. This reduced repition between the two methods. See part 5.

5. Line 64: The if condition can be simplified.

Origional Code:

invalid_penalty_per_unit = params[:late_policy][:max_penalty].to_i < params[:late_policy][:penalty_per_unit].to_i
    flash[:error] = 'The maximum penalty cannot be less than penalty per unit.' if invalid_penalty_per_unit
    same_policy_name = false
    # penalty name should be unique
    if same_policy_name != LatePolicy.check_policy_with_same_name(params[:late_policy][:policy_name], instructor_id)
      flash[:error] = 'A policy with the same name already exists.'
      same_policy_name = true
    end
    # maximum penalty cannot be greater than equal to 100
    if params[:late_policy][:max_penalty].to_i >= 100
      flash[:error] = 'Maximum penalty cannot be greater than or equal to 100'
      invalid_max_penalty = true
    end

    if !invalid_penalty_per_unit && !same_policy_name && !invalid_max_penalty
      @late_policy = LatePolicy.new(late_policy_params)
      @late_policy.instructor_id = instructor_id
      begin
        @late_policy.save!
        flash[:notice] = 'The late policy was successfully created.'
        redirect_to action: 'index'
      rescue StandardError
        flash[:error] = 'The following error occurred while saving the late policy: '
        redirect_to action: 'new'
      end
    else
      redirect_to action: 'new'
    end

Modifications:

 def validate_input(is_update=false)
    # Validates input for create and update forms
    max_penalty = params[:late_policy][:max_penalty].to_i
    penalty_per_unit = params[:late_policy][:penalty_per_unit].to_i

    valid_penalty, error_message = true, nil
    valid_penalty, error_message = duplicate_name_check(is_update)
    prefix = is_update ? "Cannot edit the policy. " : ""

    #This check validates the maximum penalty.
    if max_penalty < penalty_per_unit
      error_message = prefix + 'The maximum penalty cannot be less than penalty per unit.'
      valid_penalty = false
    end

    #This check validates the penalty per unit for a late policy.
    if penalty_per_unit < 0
      error_message = 'Penalty per unit cannot be negative.'
      valid_penalty = false
    end

    # This checks maximum penalty does not exceed 100.
    if max_penalty >= 100
      error_message = prefix + 'Maximum penalty cannot be greater than or equal to 100'
      valid_penalty = false
    end

    return valid_penalty, error_message
  end

6. Improve the variable name for same_policy_name. (Boolean variables can be named in a better way)

Current identifiers names:

same_policy_name

Modifications:

Got rid of this identifier by replacing it with a function named "duplicate_name_check"

7. Add method comments

No comments

Modifications:

Added comments for all the methods to increase the readability of the code.

GitHub links

Link to Expertiza repository: here

Link to the forked repository: here