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 Function

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 validate_input method is quite lengthy and has multiple conditions checking. It might be worth breaking down this method into smaller methods each handling a specific validation.

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.