CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller: Difference between revisions
No edit summary |
|||
Line 36: | Line 36: | ||
==Proposed Changes== | ==Proposed Changes== | ||
=== Make all file scope methods private === | |||
=== Removal of unneeded HTML POST === | |||
===Refactor children_node_ng=== | ===Refactor children_node_ng=== | ||
Line 145: | Line 146: | ||
===Remove unused functions=== | ===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). | 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). Other became unused once refactoring was completed. | ||
# filter() | |||
# filter_node_is_qan() | |||
# courses_assignments_obj() | |||
# confirm_notifications_access() | |||
# child_nodes_from_params() | |||
# bridge_to_is_available() | |||
# action() | |||
====Changes to tree_display_controller.rb==== | ====Changes to tree_display_controller.rb==== | ||
Line 164: | Line 172: | ||
[[File:Diff_config_routes_remove_functions.PNG]] | [[File:Diff_config_routes_remove_functions.PNG]] | ||
=== Assume an object with FolderNode properties is passed to children_node_2_ng === | |||
=== Populate JSON object within serialize methods === | |||
==Code Coverage== | ==Code Coverage== |
Revision as of 02:28, 23 March 2020
E2006. Refactor tree_display_controller.rb
About Expertiza
Expertiza is a web application through which students can submit and peer-review learning objects (articles, code, web sites, etc). The Expertiza project is supported by the National Science Foundation.
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:
You will see here that assignments are nested within courses. This means that the node for an assignment has a parent_id that points to a node for a course. Similarly, listings of questionnaires 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 is 379 lines long, and contains 45 methods. Many of those methods have similar names, yet there may be good reasons for having separate methods. Look over how these methods are used, and then decide. Then analyze the rest of the methods and try to find ways to save code and avoid DRY issues. Even if you cannot find a way to improve a method, you should be able to come up with a 1-line or 2-line comment explaining what that method does.
Goals
From the problem background, we have derived three main goals to focus on in the 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 an 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.
Proposed Changes
Make all file scope methods private
Removal of unneeded HTML POST
Refactor children_node_ng
We determined that the children_node_ng method had a misleading name for its function. We propose that the method be renamed to get_folder_contents since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. We believe that the proposed name change and comment make the code easier to understand (Goal 3). In addition to the name change, the get_folder_contents method is refactored to make the method itself more understandable. We also changed the method from a post to a get, to better represent what it was doing, so changes had to be made to account for the name change and method type change. In total, changes were made to tree_display.jsx, tree_display_controller.rb, and routes.rb.
Changes to tree_display_controller.rb
Changes to tree_display.jsx
Changes to routes.rb
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, so we decided that those 11 methods could be condensed into single line methods with one overarching comment to describe them. We believe that 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).
Original Implementation
# refactored method to provide direct access to parameters def goto_controller(name_parameter) node_object = TreeFolder.find_by(name: name_parameter) session[:root] = FolderNode.find_by(node_object_id: node_object.id).id redirect_to controller: 'tree_display', action: 'list' end # direct access to questionnaires def goto_questionnaires goto_controller('Questionnaires') end # direct access to review rubrics def goto_review_rubrics goto_controller('Review') end # direct access to metareview rubrics def goto_metareview_rubrics goto_controller('Metareview') end # direct access to teammate review rubrics def goto_teammatereview_rubrics goto_controller('Teammate Review') end # direct access to author feedbacks def goto_author_feedbacks goto_controller('Author Feedback') end # direct access to global survey def goto_global_survey goto_controller('Global Survey') end # direct access to surveys def goto_surveys goto_controller('Assignment Survey') end # direct access to course surveys def goto_course_surveys goto_controller('Course Survey') end # direct access to courses def goto_courses goto_controller('Courses') end def goto_bookmarkrating_rubrics goto_controller('Bookmarkrating') end # direct access to assignments def goto_assignments goto_controller('Assignments') end
Proposed Implementation
# The goto_ methods listed below are used to traverse the menu system. It is # hard to tell exactly where they are called from, but at least some (if not all) # are necessary. These functions may be better suited for another controller. def goto_controller(name_parameter) node_object = TreeFolder.find_by(name: name_parameter) session[:root] = FolderNode.find_by(node_object_id: node_object.id).id redirect_to controller: 'tree_display', action: 'list' end def goto_questionnaires; goto_controller('Questionnaires') end def goto_review_rubrics; goto_controller('Review') end def goto_metareview_rubrics; goto_controller('Metareview') end def goto_teammatereview_rubrics; goto_controller('Teammate Review') end def goto_author_feedbacks; goto_controller('Author Feedback') end def goto_global_survey; goto_controller('Global Survey') end def goto_surveys; goto_controller('Assignment Survey') end def goto_course_surveys; goto_controller('Course Survey') end def goto_courses; goto_controller('Courses') end def goto_bookmarkrating_rubrics; goto_controller('Bookmarkrating') end def goto_assignments; goto_controller('Assignments') end
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). Other became unused once refactoring was completed.
- filter()
- filter_node_is_qan()
- courses_assignments_obj()
- confirm_notifications_access()
- child_nodes_from_params()
- bridge_to_is_available()
- action()
Changes to tree_display_controller.rb
The bridge_to_is_available method is not called anywhere else in the app.
def bridge_to_is_available user = session[:user] owner_id = params[:owner_id] is_available(user, owner_id) end
Changes to routes.rb
We removed the call for bridge_to_is_available and found that the action method was also unused.
Assume an object with FolderNode properties is passed to children_node_2_ng
Populate JSON object within serialize methods
Code Coverage
To be completed.
Team Information
Brooks Anderson
Rick Holloway
PJ Loheide
Special thanks to our mentor and professor: Dr. Edward Gehringer
References
To be completed.