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 Functions

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)
    redirect_to_policy('new')
  end
  # If penalty  is valid then tries to update and save.
  if valid_penalty
    create_new_late_policy(late_policy_params)
    save_late_policy
    redirect_to_policy('index')
  # If any of above checks fails, then redirect to create a new late policy again.
  else
    redirect_to_policy('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 error_message
    handle_error(error_message)
    redirect_to_policy('edit')
  # If there are no errors, then save the record.
  else
    begin
      penalty_policy.update_attributes(late_policy_params)
      save_late_policy
      redirect_to_policy('index')
    # If something unexpected happens while updating, then redirect to the edit page of that policy again.
    rescue StandardError
      handle_error('The following error occurred while updating the late policy: ')
      redirect_to_policy('edit')
    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
  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
    # 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.
    handle_error('The following error occurred while saving the late policy: ')
    redirect_to_policy('new')
  end
end

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

# A method to extrapolate out the redirecting to policy controller states
def redirect_to_policy(location)
  if location == "edit"
    # If the location is the edit screen, use the old id that was inputted
    redirect_to action: location, id: params[:id]
  else
    redirect_to action: location
  end
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 and redirect_to_policy function.