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

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:

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.

Proposed Changes

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) [1]

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) [2]

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). Others became unused once refactoring was completed.

• filter()
• filter_node_is_qan()
• courses_assignments_obj()
• confirm_notifications_access()
• child_nodes_from_params()
• res_node_for_child()
• bridge_to_is_available()
• update_tmp_obj()
• action()

Changes to tree_display_controller.rb

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

  def filter	
    qid = 'filter+'	
    search = params[:filter_string]	
    filter_node = params[:filternode]	
    if filter_node == 'QAN'                       # Questionaire Assignment Name	
      qid = filter_node_is_qan(search, qid)	
    elsif filter_node == 'ACN'                    # Assignment Course Name	
      session[:root] = 2	
      qid << search	
    end	
    qid	
  end

  # if filter node is 'QAN', get the corresponding assignment questionnaires	
  def filter_node_is_qan(search, qid)	
    assignment = Assignment.find_by(name: search)	
    if assignment	
      assignment_questionnaires = AssignmentQuestionnaire.where(assignment_id: assignment.id)	
      if assignment_questionnaires	
        assignment_questionnaires.each { |q| qid << "#{q.questionnaire_id}+" }	
        session[:root] = 1	
      end	
    end	
    qid	
  end

The course_assignments_obj method is not called anywhere else in the app.

  def courses_assignments_obj(node_type, tmp_object, node)	
    update_tmp_obj(tmp_object, node)	
    # tmpObject["private"] = node.get_private	
    instructor_id = node.get_instructor_id	
    ## if current user's role is TA for a course, then that course will be listed under his course listing.	
    update_in_ta_course_listing(instructor_id, node, tmp_object)	
    update_instructor(tmp_object, instructor_id)	
    update_is_available(tmp_object, instructor_id, node)	
    assignments_method(node, tmp_object) if node_type == "Assignments"	
  end

The confirm_notifications_access method is not called anywhere else in the app.

  def confirm_notifications_access	
    redirect_to controller: :notifications, action: :list if current_user.try(:student?)	
  end

The child_nodes_from_params method is not called anywhere else in the app

  # finding out child_nodes from params	
  def child_nodes_from_params(child_nodes)	
    if child_nodes.is_a? String and !child_nodes.empty?	
      JSON.parse(child_nodes)	
    else	
      child_nodes	
    end	
  end

The res_node_for_child method is not called anywhere else in the app.

  # getting result nodes for child	
  # Changes to this method were done as part of E1788_OSS_project_Maroon_Heatmap_fixes	
  #	
  # courses_assignments_obj method makes a call to update_in_ta_course_listing which	
  # separates out courses based on if he/she is the TA for the course passed	
  # by marking private to be true in that case	
  #	
  # this also ensures that instructors (who are not ta) would have update_in_ta_course_listing	
  # not changing the private value if he/she is not TA which was set to true for all courses before filtering	
  # in update_tmp_obj in courses_assignments_obj	
  #	
  # below objects/variable names were part of the project as before and	
  # refactoring could have affected other functionalities too, so it was avoided in this fix	
  #	
  # fix comment end	
  #	
  def res_node_for_child(tmp_res)	
    res = {}	
    tmp_res.each_key do |node_type|	
      res[node_type] = []	
      tmp_res[node_type].each do |node|	
        tmp_object = {	
          "nodeinfo" => node,	
          "name" => node.get_name,	
          "type" => node.type	
        }	
        courses_assignments_obj(node_type, tmp_object, node) if %w[Courses Assignments].include? node_type	
        res[node_type] << tmp_object	
      end	
    end	
    res	
  end

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

The update_tmp_obj method was used only once in the app, and it was in the tree display controller. We decided that 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 seemed like it was only making minor changes to the json.

  def update_tmp_obj(tmp_object, node)	
    tmp = {	
      "directory" => node.get_directory,	
      "creation_date" => node.get_creation_date,	
      "updated_date" => node.get_modified_date,	
      "institution" => Institution.where(id: node.retrieve_institution_id),	
      "private" => node.get_instructor_id == session[:user].id	
    }	
    tmp_object.merge!(tmp)	
  end

Changes to routes.rb

We removed the call for bridge_to_is_available and found that the action method was also unused.

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.