CSC/ECE 517 Fall 2023 - E2382. Optimizing the LatePoliciesController

Introduction

The late_policies_controller.rb class houses the LatePoliciesController that controls the CRUD operations on late policies. However, there are many problems with the current implementation of this controller. In its current state, many functions are too long and repetitive as well as having inadequate variable names, comments, and error messages. This controller would benefit with optimizing its functions and various other aspects of the file.

About the LatePoliciesController

The LatePoliciesController provides CRUD functions to create, read, update, and destroy late policies. These include the index, show, new, edit, create, update, and destroy functions. Other functions are provided to allow it to work seamlessly within the framework of the overall project, including the action_allowed?, duplicate_name_check, validate_input, and various parameter and input functions. These additional functions are helper functions that ensure that the late policy can be created or updated based on if the user has the required permissions, doesn't enter a duplicate name, and inputs valid information to the late policy.

Requirements

• Refactor Long Methods: Longer functions should be refactored into smaller sub-functions to improve readability and make it easier for future alterations of the code.
• Improve Comments: More comments should be added to allow for users to easily follow through a given function and understand the specifics of its code statements.
• Follow the DRY Principle: Repeated code should be removed or moved into helper functions to allow for reusability of common code.
• Improve Testing: More tests should be created to ensure that everything works as intended and no unexpected errors occur, either exceptions or errors in logic.
• All changes must be done without the addition of new gems and must be clearly documented.

Functions to Optimize

• create: This function will be broken down into smaller functions to allow a more readable creation of new late policies. The error handling will also be altered to improve readability.
• update: Various comments will be added to the function as well as breaking down the code used for saving the late policy into a helper method to shorten the update method and make it more intuitive.
• duplicate_name_check: This function has various separate if statements that check for various things. These if statements will be broken down into separate helper methods to check for each individually. The duplicate_name_check function will be the main function that calls the various sub-functions so that it is clear what is being checked at a given step.
• validate_input: Similar to the duplicate_name_check function, this function has various if statements that can be refactored into smaller functions to check each input individually.
• Tests: Tests for creating and updating new late policies will be created. These tests will check for invalid inputs, correct error messages, etc. as well as ensure that edge cases are also captured correctly by the controller. New tests will also be created to ensure that previous functions, like the read and destroy functions, work correctly.

Create and Update

# Create method can create a new late policy.
# There are few check points before creating a late policy which are written in the if/else statements.
def create
  # First this function validates the input then save if the input is valid.
  valid_penalty, error_message = validate_input
  if error_message
    flash[:error] = error_message
  end
  # If penalty  is valid then tries to update and save.
  if valid_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'
    # If something unexpected happens while saving the record in to database then displays a flash notice and redirect to create a new late policy again.
    rescue StandardError
      flash[:error] = 'The following error occurred while saving the late policy: '
      redirect_to action: 'new'
    end
  # If any of above checks fails, then redirect to create a new late policy again.
  else
    redirect_to action: 'new'
  end
end

# Update method can update late policy. There are few check points before updating a late policy which are written in the if/else statements.
def update
  penalty_policy = LatePolicy.find(params[:id])
  # First this function validates the input then save if the input is valid.
  _valid_penalty, error_message = validate_input(true)
  if error_message
    flash[:error] = error_message
    redirect_to action: 'edit', id: params[:id]
  # If there are no errors, then save the record.
  else
    begin
      penalty_policy.update_attributes(late_policy_params)
      penalty_policy.save!
      LatePolicy.update_calculated_penalty_objects(penalty_policy)
      flash[:notice] = 'The late policy was successfully updated.'
      redirect_to action: 'index'
    # If something unexpected happens while updating, then redirect to the edit page of that policy again.
    rescue StandardError
      flash[:error] = 'The following error occurred while updating the late policy: '
      redirect_to action: 'edit', id: params[:id]
    end
  end
end

In the create and update methods, there are multiple if-else conditions and error handling which can be refactored for better readability and maintainability. The above code snippets represent the create and update functions. Both are very long and could benefit from splitting up the duplicated code and simplification.

Changes

These functions were refactored into multiple new helper functions. They were created in the hopes to reduce the amount of repeated code as well as increase the readability and maintainability of said functions.

Create: The create function has been simplified to improve its readability and size.

# Create method can create a new late policy.
# There are few check points before creating a late policy which are written in the if/else statements.
def create
  # First this function validates the input then save if the input is valid.
  valid_penalty, error_message = validate_input
  flash[:error] = error_message if error_message
  # If penalty  is valid then tries to update and save.
  begin
    if valid_penalty
      @late_policy = LatePolicy.new(params)
      @late_policy.instructor_id = instructor_id
      valid_penalty = save_late_policy
    end
  rescue StandardError
    valid_penalty = false
  end
  # Redirect to new if there's an error, index if not
  redirect_to action: (valid_penalty ? 'index' : 'new')
end

Update: The update function received a similar improvement, being shortened and simplified.

# Update method can update late policy. There are few check points before updating a late policy which are written in the if/else statements.
def update
  penalty_policy = LatePolicy.find(params[:id])
  # First this function validates the input then save if the input is valid.
  valid_penalty, error_message = validate_input(true)
  if !valid_penalty
    flash[:error] = error_message
    redirect_to action: 'edit', id: params[:id]
  # If there are no errors, then save the record.
  else
    penalty_policy.update_attributes(late_policy_params)
    error_thrown = save_late_policy(true)
    # If there was an error thrown, go back to edit, otherwise go to index
    if error_thrown
      redirect_to action: 'edit', id: params[:id]
    else
      redirect_to action: 'index'
    end
  end
end

Both create and update have been refactored and split up into various helper functions, many of them to help reduce duplicate code and size in their caller.

# Saves the late policy called from create or update
def save_late_policy(from_update = false)
  begin
    @late_policy.save!
    # If the method that called this is update
    LatePolicy.update_calculated_penalty_objects(penalty_policy) if from_update
    # The code at the end of the string gets the name of the last method (create, update) and adds a d (created, updated)
    flash_for_save(from_update)
  rescue StandardError
    # If something unexpected happens while saving the record in to database then displays a flash notice
    flash[:error] = 'The following error occurred while saving the late policy: '
    return false
  end
  true
end

def flash_for_save(from_update = false)
  flash[:notice] = "The late policy was successfully #{from_update ? 'updated' : 'created'}."
end

The save_late_policy function has been refactored from duplicate code in both create and update. Both functions had very similar code with only one line of difference as well as a few characters in strings. This function shortens both of the other functions down by separating out this duplicate code into a private function. The same can be said for both the handle_error function. The flash_for_save function also improves the cognitive complexity of the save function by taking out an if statement into another function.

Validate_input

# This function validates the input.
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 = 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

The validate_input method is quite lengthy and has multiple conditions being checked. It might be worth breaking down this method into smaller functions, each handling a specific validation to make this function more readable.

Changes

The validate_input function was originally very long and confusing. It has since been simplified down and separated into helper functions so each function only has one job.

# This function validates the input.
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
  error_messages = []
  # Validates the name is not a duplicate
  valid_penalty, name_error = duplicate_name_check(is_update)
  error_messages << name_error if name_error
  # This validates the max_penalty to make sure it's within the correct range
  if max_penalty_valid(max_penalty, penalty_per_unit)
    error_messages << "#{error_prefix(is_update)}The maximum penalty must be between the penalty per unit and 100."
    valid_penalty = false
  end
  # This validates the penalty_per_unit and makes sure it's not negative
  if penalty_per_unit_valid(penalty_per_unit)
    error_messages << 'Penalty per unit cannot be negative.'
    valid_penalty = false
  end
  [valid_penalty, error_messages.join("\n")]
end

# Validate the maximum penalty and ensure it's in the correct range
def max_penalty_valid(max_penalty, penalty_per_unit)
  max_penalty < penalty_per_unit || max_penalty > 100
end

# Validates the penalty per unit
def penalty_per_unit_valid(penalty_per_unit)
  penalty_per_unit < 0
end

# Validation error prefix
def error_prefix(is_update)
  is_update ? "Cannot edit the policy. " : ""
end

The new validate_input function has combined two of the conditions that validated the same input, max_penalty, into one helper function, max_penalty_valid. This new helper function validates everything with max_penalty. Additionally, the penalty_per_unit validation is moved to another new function. The new error_prefix function is a small helper function that obtains the prefix for specific errors, as per the original validate_input. Lastly, the original validate_input function returned the last error it found, overwriting all previous errors in favor of the last. This new validate_input returns all errors that are found in one big String.

Duplicate Name Check

if is_update
     existing_late_policy = LatePolicy.find(params[:id])
     if existing_late_policy.policy_name == params[:late_policy][:policy_name]
       should_check = false
     end
   end
   if should_check
     if LatePolicy.check_policy_with_same_name(params[:late_policy][:policy_name], instructor_id)
       error_message = prefix + 'A policy with the same name ' + params[:late_policy][:policy_name] + ' already exists.'
       valid_penalty = false
     end
   end

The above code is the current implementation of some duplication checks in this function. These if statements can be refactored into smaller sub-functions. The pseudo-code for one sub-function is as follows:

if should_check_policy_name
  Call sub-function should_check_policy_name
end

def should_check_policy_name(old valid_penalty, old error_message)
  if check policy with same name(currenty name, instructor_id)
    error_message = new error message
    valid_penalty = false
  end
  return valid_penalty, error_message

In the above sub-function, the should_check name was changed to be more explicit and the should check if-statement was refactored to be in a different method to improve readability.

Changes

The def duplicate_name_check function was originally also long not reusable according to DRY principles. It has since been simplified down and separated into helper functions so each function only has one job.

# This function checks if the policy name already exists or not and returns boolean value for penalty and the error message.
 def duplicate_name_check(is_update = false)
   valid_penalty, error_message = true, nil
   prefix = is_update ? "Cannot edit the policy. " : ""
 
   if should_check_policy_name?(is_update)
     valid_penalty, error_message = check_for_duplicate_name(prefix)
   end
   
   return valid_penalty, error_message
 end

 # This is a helper function for the duplicate name check
 def should_check_policy_name?(is_update)
   # If the function is called in the context of updating a policy
   if is_update
       # Find the existing late policy by its ID
       existing_late_policy = LatePolicy.find(params[:id])
       # Check if the policy name in the request is different from the existing policy's name
       # Return true if they are different, indicating a need to check the policy name
       return existing_late_policy.policy_name != params[:late_policy][:policy_name]
   end
   return true
 end

 # This is a helper function for the duplicate name check
 def check_for_duplicate_name(prefix)
   # Using `exists?` to check if a LatePolicy with the same name already exists for the instructor.
   # It's assumed that `params[:late_policy][:policy_name]` holds the name of the policy and 
   # `instructor_id` is the ID of the instructor.
   if LatePolicy.check_policy_with_same_name(params[:late_policy][:policy_name], instructor_id)
     error_message = prefix + 'A policy with the same name ' + params[:late_policy][:policy_name] + ' already exists.'
     valid_penalty = false
   else
     # If no duplicate name is found, set valid_penalty to true
     valid_penalty = true
     error_message = nil
   end

   # Return the validity status of the penalty and the corresponding error message, if any
   return valid_penalty, error_message
 end

The new duplicate_name_check function has been simplified down to using two helper functions to simplify the functionality of the code. It uses the should_check_policy_name and check_for_duplicate_name to make sure that there are no duplicate late policies. Each helper function has its own functionality, the first one, should_check_policy_name makes sure we need to check if there is a duplicate. The second one, check_for_duplicate_name is only used if we do need to check the duplicate and it returns whether it is valid and the error message.

Index

def index

    @penalty_policies = LatePolicy.where(['instructor_id = ? OR private = 0', instructor_id])
    respond_to do |format|
      format.html # index.html.erb
      format.xml  { render xml: @penalty_policies }
    end
  end

The above code is the current implementation of the index function. The index method can be optimized by using includes or joins to prevent N+1 query problems if there are associated records being accessed in the view.

Changes

def index

    @penalty_policies = LatePolicy.includes(:instructor).where(['instructor_id = ? OR private = 0', instructor_id])
    respond_to do |format|
      format.html
      format.xml  { render xml: @penalty_policies }
    end
  end

The main change here is the .includes() call which allows for eager loading the associated user models.

Proposed Solution

There are multiple routes we can go down to tackle this issue. One way to break down a long method would be to refactor the create method, we can extract and rewrite the save_late_policy method. We also have to refactor the duplicate name check method by rewriting the if statements as smaller sub methods. We would then have to write tests for all the new functionality/refactoring done.

Test Changes

Various duplicate code in the latest_policies_controller_spec file has been factored out into helper functions.

def create_late_policy(policy_name, max_penalty, penalty_per_unit, instructor_id) 
  late_policy = LatePolicy.new
  late_policy.policy_name = policy_name
  late_policy.max_penalty = max_penalty
  late_policy.penalty_per_unit = penalty_per_unit
  late_policy.instructor_id = instructor_id
  late_policy
end

def request_params(policy_name, max_penalty, penalty_per_unit) {
  late_policy: {
    max_penalty: max_penalty,
    penalty_per_unit: penalty_per_unit,
    policy_name: policy_name
  }
}
end

def request_params_with_id(policy_name, max_penalty, penalty_per_unit, id) {
  late_policy: {
    max_penalty: max_penalty,
    penalty_per_unit: penalty_per_unit,
    policy_name: policy_name
  },
  id: id
}
end

# Create an RSpec example to reduce code duplication
RSpec.shared_examples 'late policy creation with error' do |policy_name, max_penalty, penalty_per_unit, expected_error|
  before(:each) do
    latePolicy = LatePolicy.new
    allow(latePolicy).to receive(:check_policy_with_same_name).with(any_args).and_return(false)
  end
  it 'throws a flash error' do
    post :create, params: request_params(policy_name, max_penalty, penalty_per_unit)
    expect(flash[:error]).to eq(expected_error)
    expect(response).to redirect_to('/late_policies/new')
  end
end

These additions to the test file, late_policies_controller_spec.rb, helps reduce duplicate code in the test by refactoring the shared components into these helper methods. Each helper method sets up specific late_policies to be used in various tests in the file.