E1645: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
(More code updates)
Line 128: Line 128:
</pre>
</pre>


==3) Useless assignment to variable - `childNodes` were removed from following two places.==
==3) Useless assignment to variable - `childNodes`.==
 
Following were removed from the two places:
<pre>
<pre>
135 -  childNodes = {}
135 -  childNodes = {}
Line 137: Line 137:
222 -  childNodes = {}
222 -  childNodes = {}
</pre>
</pre>


==4) Use snake_case for variable names.==
==4) Use snake_case for variable names.==
Variable names using camelCase were renamed to use snake_case as according to ruby coding guidelines.
Variable names using camelCase were renamed to use snake_case as according to ruby coding guidelines.
Before:
Before:
<pre>
<pre>
223- childNodes  
- childNodes = {}
228- tmpRes
- tmpRes = {}
239-  nodeType
</pre>
</pre>


After:
After:
<pre>child_nodes
<pre>
tmp_res
+ child_nodes = child_nodes_from_params(params[:reactParams][:child_nodes])
node_type
+ tmp_res = {}
</pre>
</pre>


==5) Prefer `each` over `for`==
==5) Prefer `each` over `for`==
For was replace by each, as follows:
Before:
Before:
<pre>
<pre>
Line 163: Line 161:
<pre>
<pre>
</pre>
</pre>


==6)The use of `eval` is a serious security risk.==
==6)The use of `eval` is a serious security risk.==
Line 181: Line 178:


==7) Avoid the use of the case equality operator `===`.==
==7) Avoid the use of the case equality operator `===`.==
`===` has been replaced by `==` in the following manner:
<pre>
<pre>
- tmpObject["private"] = node.get_instructor_id === session[:user].id ? true : false
</pre>
</pre>
has been changed to  ⇒
has been changed to  ⇒
<pre>
<pre>
+  "private" => node.get_instructor_id == session[:user].id ? true : false
</pre>
</pre>
==8) Line is too long.==
==8) Line is too long.==
[[File:8_1_before.png]]
<pre>
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))
</pre>
Changed to  ⇒
<pre>
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))
</pre>


<pre>
- 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)
</pre>
Changed to ==>
Changed to ==>


[[File:8_1_after.png]]
<pre>
 
+  def is_current_user_ta?(instructor_id, child)
[[File:8_2_before.png]]
+    # instructor created the course, current user is the ta of this course.
 
+    session[:user].role_id == 6 and
Changed to ==>
+        Ta.get_my_instructors(session[:user].id).include?(instructor_id) and ta_for_current_course?(child)
 
  end
[[File:8_2_after.png|upright=1.5]]
</pre>
 
=Future Considerations=
=Future Considerations=


=Conclusion=
=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. 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.
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. 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.

Revision as of 22:31, 28 October 2016

E1645: Refactoring TreeDisplayController

Project Links

Github Repo
Expertize Pull Request
Travis CI Test Cases Passes


Setup - How To View Our Expertiza Fork Running on AWS

NOTE: The scope of our project was only to refactor select code. There are no feature additions or changes, so it should work just like production Expertiza.

1) Go to our Expertiza project running on AWS

2) Login as Admin
User: user2
Password: password

3) Click on "Manage..." if you are not automatically directed to the tree display.

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.

Refactoring TreeDisplayController Issues

As part of the refactoring task, we had to resolve the following issues for enhancing code readability and maintainability. The tasks included:

  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.

1) Similar code found in other locations

In order to make the code DRY, similar code was moved to a new function, and that was called from the remaining part of the code.

  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

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

By creating two separate functions child_nodes_from_params and initialize_fnode_update_children, the 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 = {}

After:

+ child_nodes = child_nodes_from_params(params[:reactParams][:child_nodes])
+ tmp_res = {}

5) Prefer `each` over `for`

For was replace by each, as follows: Before:


After:


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 `===`.

`===` 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.

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

Future Considerations

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. 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.