CSC/ECE 517 Fall 2016/E1645. Refactoring Tree Display Controller

Introduction to Expertiza

Expertiza is a peer review based system which provides incremental learning from the class. This project has been developed together by faculty and students using Ruby on Rails framework. Expertiza allows the instructor to create, edit and delete assignments, create new assignment topics, assign them to a particular class or selected students, have students work on teams and then review each other's assignments at the end. For the students, they can signup for topics, form teams, and submit their projects and assignments. Students then review the work done by other students and give suggestions to improve. Teams after reviews are allotted scores and they can refer to the peer comments to further improve their work. It also supports submission of different file types for assignments, including the URLs and wiki pages.

Project Description

For this project, our team refactored the TreeDisplayController class in the Expertiza OSS project. This class provides access to questionnaires, review rubrics, author feedback, courses, assignments, and course evaluations. The tree display lists all of these categories with the ability for the user to "drill down" into the subcategories or just expand/collapse as needed. The main objective of the project was to refactor the controller to follow good Ruby practices and improve the grade given by Code Climate by fixing issues detected without breaking any of the existing test cases or functionality of Expertiza.

Refactoring TreeDisplayController Issues

As part of the refactoring task, we had to resolve the issues detected by Code Climate for enhancing code readability and maintainability. Some of the issues detected were:

1. Similar code found in other locations.
2. Cyclomatic and Perceived complexity for get_children_node_ng is too high.
3. Useless assignment to variable - `childNodes`.
4. Use snake_case for variable names.
5. Prefer `each` over `for`.
6. The use of `eval` is a serious security risk.
7. Avoid the use of the case equality operator `===`.
8. Line is too long.
9. `end` at 334, 2 is not aligned with `class` at 1, 0.

We used the DRY principle while refactoring our code to remove duplicates in get_children_node_ng and get_children_node_2_ng methods.

1) Similar code found in other locations

Duplicated code often leads to a software that is hard to understand and even more difficult to change. According to the Don't Repeat Yourself (DRY) principle, "Every piece of knowledge must have a single, unambiguous, authoritative representation within a system" . Violating the DRY principle often leads to more bugs and maintenance problems. In order to make the code DRY, similar code was moved to a new method, and that method was called from the remaining part of the code wherever the same set of lines of code were required.

  def goto_questionnaires
    node_object = TreeFolder.find_by_name('Questionnaires')
    session[:root] = FolderNode.find_by_node_object_id(node_object.id).id
    redirect_to controller: 'tree_display', action: 'list'
  end

  # direct access to review rubrics
  def goto_review_rubrics
    node_object = TreeFolder.find_by_name('Review')
    session[:root] = FolderNode.find_by_node_object_id(node_object.id).id
    redirect_to controller: 'tree_display', action: 'list'
  end

  # direct access to metareview rubrics
  def goto_metareview_rubrics
    node_object = TreeFolder.find_by_name('Metareview')
    session[:root] = FolderNode.find_by_node_object_id(node_object.id).id
    redirect_to controller: 'tree_display', action: 'list'
  end

  # direct access to teammate review rubrics
  def goto_teammatereview_rubrics
    node_object = TreeFolder.find_by_name('Teammate Review')
    session[:root] = FolderNode.find_by_node_object_id(node_object.id).id
    redirect_to controller: 'tree_display', action: 'list'
  end

  # direct access to author feedbacks
  def goto_author_feedbacks
    node_object = TreeFolder.find_by_name('Author Feedback')
    session[:root] = FolderNode.find_by_node_object_id(node_object.id).id
    redirect_to controller: 'tree_display', action: 'list'
  end

Since only the parameter in find_by_name method was different , we have passed this as an argument in goto_controller method which then executes the same set of steps wherever required with different parameters.

Changed to ⇒

 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

2) Cyclomatic and Perceived complexity for get_children_node_ng is too high

Code Climate has a predefined maximum cyclomatic complexity and raises Cyclomatic complexity too high issue whenever a method exceeds this predefined maximum complexity.This cop tries to produce a complexity score that's a measure of the complexity the reader experiences when looking at a method and raises a Perceived complexity too high issue whenever a method exceeds a preset maximum. In contrast to the CyclomaticComplexity cop, this cop considers else nodes as adding complexity.

By creating two separate functions child_nodes_from_params and initialize_fnode_update_children,both the cyclomatic and perceived complexity for this function was reduced. The initialize_fnode_update_children function was in-turn sub-functioned into update_fnode_children which updated the children nodes. The new function looks like this:

  def children_node_ng
    child_nodes = child_nodes_from_params(params[:reactParams][:child_nodes])
    tmp_res = {}
    child_nodes.each do |node|
      initialize_fnode_update_children(params, node, tmp_res)
    end
    # res = res_node_for_child(tmp_res)
    res = res_node_for_child(tmp_res)
    respond_to do |format|
      format.html { render json: res }
    end
  end

3) Useless assignment to variable - `childNodes`.

Following were removed from the two places:

135 -  childNodes = {}

222 -  childNodes = {}

4) Use snake_case for variable names.

Variable names using camelCase were renamed to use snake_case as according to ruby coding guidelines.

Before:

- childNodes = {}
- tmpRes = {}
- nodeType = child.type

After:

+ child_nodes = child_nodes_from_params(params[:reactParams][:child_nodes])
+ tmp_res = {}
+ node_type = child.type

5) Prefer `each` over `for`

The following for loops were replaced by each, as follows: Before:

- 143    for node in childNodes
- 146    for a in node
- 159    for nodeType in tmpRes.keys
- 162    for node in tmpRes[nodeType]
- 238   for child in tmpRes

After:

+ child_nodes.each do |node|
+ node.each do |a|
+ tmp_res.keys.each do |node_type|
+ tmp_res[node_type].each do |node|
+ tmp_res.each do |child|

6)The use of `eval` is a serious security risk.

eval() executes a string of characters as code. eval() is used in situations where the string contents are not known beforehand or even when the string is generated dynamically. Basically, when eval() is called , it starts a compiler to translate the string. This is detrimental especially when eval() is called on a string submitted by or modifiable by the user. Imagine if the string contains an OS call like rm -rf. But , it is perfectly safe to use eval() on strings , if they are known to be constrained. This problem is analogous to SQL injection.

Before:

- for node in childNodes
-      fnode = eval(params[:reactParams][:nodeType]).new

After:

+  def initialize_fnode_update_children(params, node, tmp_res)
+    fnode = (params[:reactParams][:nodeType]).constantize.new

7) Avoid the use of the case equality operator `===`.

As per good Ruby practices, the case equality operator is used only internally along with case statement and should not be abused where simpler operators would do the trick , so as to produce highly readable and easily digestible code. Hence `===` has been replaced by `==` in the following manner:

- tmpObject["private"] = node.get_instructor_id === session[:user].id ? true : false

has been changed to ⇒

+  "private" => node.get_instructor_id == session[:user].id ? true : false

8) Line is too long.

Very long lines needs to be made short so that it is more readable, which was done in the following manner:

tmpObject["is_available"] = is_available(session[:user], instructor_id) || (session[:user].role.ta? && Ta.get_my_instructors(session[:user].id).include?(instructor_id) && ta_for_current_course?(node))

Changed to ⇒

tmpObject["is_available"] = is_available(session[:user], instructor_id) || (session[:user].role.ta? && Ta.get_my_instructors(session[:user].id).include?(instructor_id) && ta_for_current_course?(node))

- available_condition_2 = session[:user].role_id == 6 and Ta.get_my_instructors(session[:user].id).include?(instructor_id) and ta_for_current_course?(child)

Changed to ==>

+  def is_current_user_ta?(instructor_id, child)
+    # instructor created the course, current user is the ta of this course.
+    session[:user].role_id == 6 and
+        Ta.get_my_instructors(session[:user].id).include?(instructor_id) and ta_for_current_course?(child)
  end

Run the test cases

Follow the following steps to run all the Rspec tests associated with tree_display_controller

1.) git clone https://github.com/shubham2892/expertiza.git

2.) cd expertiza

3.) bundle install

4.) rename database.yml.example to database.yml and update the database credentials

5) rename secrets.yml.example to secret.yml

6) bundle install

7.) rspec spec/controllers/tree_display_controller_spec.rb

How to test the changes from UI

• Login in as an instructor or admin using credentials (admin with password: admin or instructor6 with password: password)
• To check the changes made in goto_controller method, hover over the Manage tab in the navigation bar on the top and click all the links and check whether they are being redirected to correct pages. For example, if Questionnaires is clicked, you should be redirected directly to the page displaying all the questionnaires with its sub-categories like Reviews, Surveys etc.
• When you click on either of the parent nodes - Assignments,Questionnaires and Courses , all the Assignments , Questionnaires and Courses concerned to the Instructor are displayed.
• To test the changes made in get_children_node_ng and get_children_node_2_ng methods, click on the sub categories under each of the parent tree displays (Courses and Questionnaires) and all of them will further expand to show the details (Click on the name of the Course/Questionnaire to expand them), i.e when you click on a particular course , all the assignments that belong to the corresponding course are displayed.

Direct access to sub categories in tree display

Checking the direct access tabs from navigation bar

Checking the sub categories in each parent Tree

Conclusion

In conclusion, we were able to successfully refactor the tree_display_controller and related Rspec tests. We have improved and expanded the use of RESTful and DRY design choices, and overall improved the quality and efficiency of the Expertiza codebase. Since a lot of duplicated code was removed, the overall test coverage of Expertiza has also been incresed. We have fixed all the issues detected by Code Climate Chrome Extension and refactored the code to make sure it followed good ruby practices and in the end were able to improve the Code Climate score from F to A while not breaking any of the Rspec tests related.

From:

To:

Project Links

Expertiza fork running on VCL Github Repo
Expertize Pull Request
Travis CI Test Cases Passed