CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

Project Description

Problem Statement

E2006. Refactor tree_display_controller.rb

Background

The tree_display_controller is responsible for rendering hierarchical lists in the instructor user interface. Here is a sample instructor homepage:

The image below shows that specific questionnaires are nested under different questionnaire categories. This means that the node for a questionnaire has a parent_id that points to a node for a questionnaire category. Similarly, listings of courses and their assignments are hierarchical.

The various rubrics, as well as surveys, are types of questionnaires. So the parent_id of a particular rubric will be the type of rubric (e.g., teammate review) that it is.

The tree_display_controller was 379 lines long, and contained 45 methods. Many of those methods had similar names and were performing very similar functions. In an effort to save code and avoid DRY issues this file was refactored.

This effort reduced the controller to less than 200 lines, 20 public methods (15 of which are necessary scaffold for menu navigation), and 6 private methods.

Goals

From the problem background three main goals were derived to focus on in the refactor of tree display controller:

Remove unnecessary methods

Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.

Save code and avoid DRY issues

In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller and easier to read.

Improve understanding

For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.

Changes

The sections below include a link to the commits where the changes were made following the description. All code changes can be found in pull request #1698

Make all file scope methods private

Code that is only called within the tree_display_controller.rb was moved to the private scope. This makes the code easier to read and easier to understand what is getting called outside the controller. Assign private designation to internal methods

Combine folder_node_ng_getter with children_node_ng

folder_node_ng_getter was being called by the view to return FolderNode.get. The resulting json of this call was then passed, without modification, into children_node_ng. Since this was the only place folder_node_ng_getter was being called it was combined with children_node_ng to make things easier to read when when viewing controller code. Refactor children_node_ng

Refactor children_node_ng

children_node_ng was a misleading name. The method has been renamed get_folder_contents since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. In addition to the name change, the get_folder_contents method was refactored to include the behavior of folder_node_ng_getter. This addition simplifies the code because the method no longer needs to convert the object that was converted to json back to an object. A single call serialize_folder_to_json makes the code easier to read instead of chasing nested function calls through initialize_fnode_update_children which ultimately leads to the same result. Another key change was changing the method from a post to a get, to better represent what it was doing. Changes to tree_display.jsx and routes.rb had to be made to account for the name change and method type change. Refactor children_node_ng

Refactor children_node_ng_2

children_node_ng_2 was a misleading name. The method has been renamed get_sub_folder_contents since it returns the subfolder items for the folders obtained by get_folder_contents in json format. Like the get_folder_contents refactor get_sub_folder_contents was simplified to make a single call to serialize_sub_folder_to_json making the code easier to read. Again this method was changed from a post to a get, to better represent what it was doing. Changes to tree_display.jsx and routes.rb had to be made to account for the name change and method type change. Refactor children_node_ng_2 Rename children_node_2_ng to get_sub_folder_contents Rename serialize_to_json to serialize_sub_folder_to_json

Inline function calls to make json serialization more clear

Where it could be done without making the existing functions unwieldy functions that updated json properties were moved up one level. This makes it easier to see in one place what properties are being modified by the serialization functions. Inline function calls to make json serialization more clear

Collapse 'goto' methods

In the legacy code found in tree_display_controller.rb, 60 lines are dedicated to the 12 methods related to traversing the menu system. 11 of the methods call the goto_controller() method with different values. Those 11 methods were condensed into single line methods with one overarching comment to describe them. This change makes the code easier to read, since each of the 11 methods condensed carry out the same functionality, and easier to understand since we have added in a better comment for understanding what the code does (Goal 3). This also saved 41 lines of code in the controller (Goal 2). Compact gotos

Remove unused functions

By searching for the implementations of all functions in the controller we were able to recognize some of them as being unused and remove them (Goal 1). Others became unused once refactoring was completed.

• filter()
• filter_node_is_qan()

These methods were both removed because they were only used in the page_header partial for tree display, which was removed approximately 3 years prior.

• courses_assignments_obj()
• confirm_notifications_access()
• child_nodes_from_params()
• res_node_for_child()

These methods are not used in the app.

• update_tmp_obj()

This method was used only once in the app, and it was in the tree display controller, since the purpose of the method was aligned with the purpose of the serialize_to_json method which called it there was no reason to write a new method to call. Especially since the update_tmp_obj method was only making minor changes to the json.

• bridge_to_is_available()
• action()

These methods are not called in the app. References to these methods were removed from routes.rb

Team Information

Brooks Anderson

Rick Holloway

PJ Loheide

Special thanks to our mentor and professor: Dr. Edward Gehringer