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 Functions

# 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.

Validate_input Function

# 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.

Duplicate Name Check Function

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.

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.

Create and Update

The Create and Update functions were refactored into multiple new helper functions. These functions 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
  if error_message
    handle_error(error_message)
  end
  # If penalty  is valid then tries to update and save.
  if valid_penalty
    create_new_late_policy(late_policy_params)
    error_thrown = save_late_policy
    if error_thrown
      # If there was an error thrown go to new, otherwise go to index
      redirect_to action: 'new'
    else
      redirect_to action: 'index'
    end
  # If any of above checks fails, then redirect to create a new late policy again.
  else
    redirect_to action: 'new'
  end
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
    handle_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
    # 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.

# Create and save the late policy with the required params
def create_new_late_policy(params)
  @late_policy = LatePolicy.new(params)
  @late_policy.instructor_id = instructor_id
end

# Saves the late policy called from create or update
def save_late_policy
  error_thrown = false
  begin
    @late_policy.save!
    if caller_locations(2,1)[0].label == 'update'
      # If the method that called this is update
      LatePolicy.update_calculated_penalty_objects(penalty_policy)
    end
    # The code at the end of the string gets the name of the last method (create, update) and adds a d (created, updated)
    flash[:notice] = "The late policy was successfully #{caller_locations(2,1)[0].label}d."
  rescue StandardError
    error_thrown = true
    # 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.
    if caller_locations(2,1)[0].label == 'update'
      message_thrown = 'The following error occurred while updating the late policy: '
    else
      message_thrown = 'The following error occurred while saving the late policy: '
    end
    handle_error(message_thrown)
  end
  error_thrown
end

# A method to extrapolate out the flashing of error messages
def handle_error(error_message)
  flash[:error] = error_message
end

The create_new_late_policy function is refactored from the create method to move all the code required to create a new blank late policy into one method. This slightly improves readability in the create method. 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.

Validate_input

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_validation(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_validation(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_validation(max_penalty, penalty_per_unit)
  max_penalty < penalty_per_unit || max_penalty > 100
end

# Validates the penalty per unit
def penalty_per_unit_validation(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_validation. 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.

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