CSC/ECE 517 Fall 2014/oss E1463 vpd: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
 
(45 intermediate revisions by 2 users not shown)
Line 8: Line 8:
[https://github.com/expertiza/expertiza/pull/449 Expertize Pull Request] <br>
[https://github.com/expertiza/expertiza/pull/449 Expertize Pull Request] <br>
[http://ec2-54-186-80-176.us-west-2.compute.amazonaws.com:3000/ Project Running on AWS]<br>
[http://ec2-54-186-80-176.us-west-2.compute.amazonaws.com:3000/ Project Running on AWS]<br>
Admin Account login details : <br>
 
Username: user2 <br>
=Setup - How To View Our Expertiza Fork Running on AWS=
Password: password
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 [http://ec2-54-186-80-176.us-west-2.compute.amazonaws.com:3000/ our Expertiza project running on AWS]
 
2) Login as Admin<br>
'''User:''' user2<br>
'''Password:''' password
 
3) Click on "Manage..." if you are not automatically directed to the tree display.


=Project Description=
=Project Description=
Line 22: Line 30:
=Refactoring TreeDisplayController Tasks=
=Refactoring TreeDisplayController Tasks=


As part of the refactoring task, we had to address the following issues for enhancing code readability and maintainability. The tasks include-
As part of the refactoring task, we had to address the following issues for enhancing code readability and maintainability. The tasks included:
#Changing List to Index using a [http://www.restapitutorial.com/lessons/whatisrest.html RESTful] approach.
#Changing List to Index using a [http://www.restapitutorial.com/lessons/whatisrest.html RESTful] approach.
#Use of routing helpers
#Combined all of the goto methods
#Use {} and [] instead of Hash.new and Array.new
#Use of [http://guides.rubyonrails.org/routing.html#path-and-url-helpers routing helpers]
#Ensuring that instantiation of instance variables is minimized.
#Ensuring that instantiation of instance variables is minimized.


In the end, we enhanced this list a bit and resulted in doing the following:
#Changed `list` method to `index`
#Removed all of the `goto` methods and `drill`, and had them go through `index` instead because they are essentially just filtered versions of a listing (and the previous implementation just redirected each `goto` back to `list` anyway)
#Used path helpers where possible
#Removed instance variables and used local variables instead.  This should be better for scope reasons.  Note that there are still 1-2 instance variables used in the view, `@show` and `@assignment` which don't appear to be used anywhere.  (`@show` doesn't seem to ever be set, `@assignment` seems to only be used locally)  However, we didn't touch this because it didn't deal directly with our changes to the controller.
#Introduced some local variables as params to the view which allow dropdowns to keep their selected option after the page reloads (the logic was there before, but it was using instance variables which were never set!). This is for the search/filtering options on the "Manage..." screen.


==Changing List to Index==
==1) Changing List to Index==
All our code changes can be checked out at our [https://github.com/Druotic/expertiza/pull/1/files Git project repository].


==Combined All of the Goto Methods==
In order to make the codebase more [http://www.restapitutorial.com/lessons/whatisrest.html RESTful], we have replaced all the controller action redirections with a tree_display_index_path which enormously reduces the code content and as well as provides an organized approach to action redirections.
 
[[File:List_to_Index.png]]
 
As seen below, we were able to '''remove''' list from Routes.rb:
<pre>
resources :tree_display do
collection do
  get ':action'
    - post 'list'
  end
end
</pre>
 
==2) Combined All of the Goto Methods==
We also combined all of the goto methods into one, in accordance with the [http://en.wikipedia.org/wiki/Don't_repeat_yourself DRY] principle. This removed a significant amount of duplicate code.
We also combined all of the goto methods into one, in accordance with the [http://en.wikipedia.org/wiki/Don't_repeat_yourself DRY] principle. This removed a significant amount of duplicate code.


3 of the 10 unnecessary methods removed from tree_display_controller.rb
Shown below are 3 of the 10 very similar methods we removed from '''tree_display_controller.rb''':
<pre>
<pre>
- # direct access to questionnaires  
- # direct access to questionnaires  
Line 59: Line 86:
</pre>
</pre>


These methods were replaced by a variable containing a list mapping of all the session variable strings to database labels. The goto_assignments method was replaced with an index method to accept this variable, rather then calling each goto method individually.
These methods were replaced by a variable containing a list mapping all the session variable strings to database labels. The goto_assignments method was replaced with an index method to accept this variable, rather then calling each goto method individually.


Added variable:
'''Added''' variable:
<pre>
<pre>
+ @@Groups = {  
+ @@Groups = {  
Line 78: Line 105:
</pre>
</pre>


'''Removed''' goto_assignments method:
<pre>
<pre>
Removed goto_assignments method:
- # direct access to assignments  
- # direct access to assignments  
- def goto_assignments  
- def goto_assignments  
Line 87: Line 114:
- end  
- end  
</pre>
</pre>
'''Added''' index method:
<pre>
<pre>
Added index method:
+ def index  
+ def index  
+  session[:root] = params[:root]  
+  session[:root] = params[:root]  
Line 98: Line 126:
</pre>
</pre>


==Use Routing Helpers==
==3) Use Routing Helpers==
We refactored the treedisplay controller class to use route helpers rather than calling ":action => 'list', :controller => 'tree_display'", or something similar. This involved many edits in the tree_display_controller.rb file and related controller and view files. Our [https://github.com/Druotic/expertiza Github repo] shows all the changes made.
We refactored the tree display controller class to use route helpers rather than calling ":action => 'list', :controller => 'tree_display'", or something similar. This involved many edits in the tree_display_controller.rb file and related controller and view files. As seen below, we utilized routing helpers and replaced '' ':controller=>'course', :action=>'new', :private => 0'' with ''new_course_path(private: 0)''.


Breadcrumbs view file before:
'''_courses_folder_actions.html.erb''' changes:
<pre>
<pre>
redirect_to :action => 'list', :controller => 'tree_display'
<ul>
  <div>
  <%= link_to image_tag('/assets/tree_view/add-public-24.png'),
  - { :controller=>'course', :action=>'new', :private => 0 },
  - { :title => 'Create Public Course', id: 'create-public-course' } %>
  + new_course_path(private: 0),
  + { title: 'Create Public Course', id: 'create-public-course' } %>
  <%= link_to image_tag('/assets/tree_view/add-private-24.png'),
  - { :controller=>'course', :action=>'new', :private => 1 },
  - { :title => 'Create Private Course', id: 'create-private-course' } %>
  + new_course_path(private: 1),
  + { title: 'Create Private Course', id: 'create-private-course' } %>
  </div>
  </ul>
  </li>
</ul>
</pre>
</pre>


After:
Another example of routing helpers in '''_bread_crumbs.html.erb'''. <br>
 
'''Before:'''
<pre>
<pre>
redirect_to direct_to tree_display_index_path
<%= link_to curr_node.get_name, :controller => 'tree_display', :action => 'drill', :root => curr_node.id %>
</pre>
</pre>


It can be seen that we have replaced all the controller action redirections with a tree_display_index_path which enormously reduces the code content and as well as provides an organized approach to action redirections.
'''After''':
 
<pre>
[[File:List_to_Index.png]]
<%= link_to curr_node.get_name, tree_display_index_path(root: curr_node.id)%>
</pre>


==Minimizing the count of Instance Variables==
==4) Minimizing the count of Instance Variables==
=== Instance Variables in Ruby ===
=== Instance Variables in Ruby ===
An instance variable has a name beginning with @, and its scope is confined to whatever object self refers to. Two different objects, even if they belong to the same class, are allowed to have different values for their instance variables. From outside the object, instance variables cannot be altered or even observed (i.e., ruby's instance variables are never public) except by whatever methods are explicitly provided by the programmer. As with globals, instance variables have the nil value until they are initialized.
An instance variable has a name beginning with @, and its scope is confined to whatever object self refers to. Two different objects, even if they belong to the same class, are allowed to have different values for their instance variables. From outside the object, instance variables cannot be altered or even observed (i.e., ruby's instance variables are never public) except by whatever methods are explicitly provided by the programmer. As with globals, instance variables have the nil value until they are initialized.
Line 123: Line 169:
We have observed that in Expertiza, there were several code files related to the Tree Display controller which were observed to be using multiple instance variables which could be reduced. Our team aimed to limit the instantiation of such variables to only necessary places in our code.
We have observed that in Expertiza, there were several code files related to the Tree Display controller which were observed to be using multiple instance variables which could be reduced. Our team aimed to limit the instantiation of such variables to only necessary places in our code.


==Other Methods==


=Summary and Conclusions=
The following was added to the ''index'' function of '''tree_display_controller.rb''' in order to replace the use of instance variables with local variables:
=Future Work=
<pre>
locals  search:      search,
        sortvar:      sortvar,
        sortorder:    sortorder,
        root_node:    root_node,
        child_nodes:  child_nodes,
        filternode:  params[:filternode],
        searchnode:  params[:searchnode]
</pre>
 
Where the ''locals'' method is defined as follows:
<pre>
private
 
  # render local variable hash for view
  # reference: http://thepugautomatic.com/2013/05/locals/
  def locals(values)
    render locals: values
  end
</pre>
 
Many of the view files were changed to make use of the local variables.
 
Here is an example of how _bread_crumbs.html.erb was changed. Note the removal of the "@" characters.
 
'''Before:'''
<pre>
<% if @root_node %>
  <%= link_to "Return to top", tree_display_index_path %>
  <%
      curr_node = @root_node
  while curr_node.parent_id
    curr_node = Node.find(curr_node.parent_id) %>
  &nbsp;&nbsp;
  <%= link_to curr_node.get_name, tree_display_index_path(root: curr_node.id)%>
<% end %>
&nbsp;&nbsp;<%= @root_node.get_name %><%
end %>
</pre>
 
'''After:'''
<pre>
<% if root_node %>
  <%= link_to "Return to top", tree_display_index_path %>
  <%
      curr_node = root_node
  while curr_node.parent_id
    curr_node = Node.find(curr_node.parent_id) %>
  &nbsp;&nbsp;
  <%= link_to curr_node.get_name, tree_display_index_path(root: curr_node.id)%>
<% end %>
&nbsp;&nbsp;<%= root_node.get_name %><%
end %>
</pre>
 
=Unexpected Database Changes=
 
Unfortunately, each method of the tree_display controller has a reference to it stored in the database, along with a route.  Essentially, the routes are being stored in the database and a wildcard is used in the routes table to allow this to happen.  The following is a sample of some of the changes made (as part of tasks 1 and 2 given above) which were required in the db/seeds.rb file:
 
<pre>
-ControllerAction.create(:site_controller_id => SiteController.find_by_name('tree_display').id, :name => 'goto_teammate_reviews', :permission_id => nil, :url_to_use => '')
-ControllerAction.create(:site_controller_id => SiteController.find_by_name('tree_display').id, :name => 'goto_metareview_rubrics', :permission_id => nil, :url_to_use => '')
-ControllerAction.create(:site_controller_id => SiteController.find_by_name('tree_display').id, :name => 'goto_teammatereview_rubrics', :permission_id => nil, :url_to_use => '')
+ControllerAction.create(:site_controller_id => SiteController.find_by_name('tree_display').id, :name => 'index', :permission_id => nil, :url_to_use => '')
</pre>
 
In addition, these name attributes are used in other tables, such as the menu_items table.  As a result, for every method in the tree_display_controller, changes like the following had to be made:
 
<pre>
MenuItem.create(:parent_id => MenuItem.find_by_name('manage/questionnaires').id, :name => 'manage/questionnaires/course evaluations', :label => 'Course evaluations', :seq => 7, :content_page_id => nil,
-                :controller_action_id => ControllerAction.where(site_controller_id: SiteController.find_by_name('tree_display').id, name:  'goto_surveys').first.id)
+                :controller_action_id => ControllerAction.where(site_controller_id: SiteController.find_by_name('tree_display').id, name:  'index').first.id)
</pre>
 
 
Instead, we believe it would be better for this to be defined in the routes file.  Having routes defined in the database is not transparent at all, and causes much confusion for developers that are new to the project - especially those which are new to Rails as well.  This should be inspected for possible future refactorings.
 
 
=Future Considerations=
 
As mentioned in the "Unexpected Database Changes" section, routing should be removed from the database and the routes.rb file should be used instead.  There are several other controllers/actions which are treated the same way.
 
In the views for tree_display, there are two instances variables which look like they could be removed - ''@show'' and ''@assignment''. ''@show'' appears to never be set anywhere, despite it being used in ''tree_display_controller'', ''_entry.html.erb'', and ''_listing.html.erb''.  Perhaps it was previously used, but then it became obsolete and was never removed.  ''@assignment'' is set inside of the view it is being used, ''actions/_assignments_actions.html.erb'', but it appears to never be used elsewhere.  In this case, ''@assignment'' should probably be replaced with a local variable.  However, we did not touch on either of these issues because they were outside the scope of our task (''@show'' is never set inside the controller and ''@assignment'' doesn't appear in the controller whatsoever).  We focused primarily on only refactoring the controller and places in the view which were affected.  In some cases, we went above and beyond and refactored additional parts of the tree_display views which were not required.  However, we had to draw a line at some point due to time constraints.
 
'''_listing.html.erb'''
<pre><% child_nodes = node.get_children(sortvar,sortorder,@show,nil,nil,search) %></pre>
 
'''_entry.html.erb'''
<pre><% child_nodes = node.get_children(sortvar,sortorder,session[:user].id,@show,search) %></pre>
 
'''actions/_assignments_actions.html.erb'''
<pre>
  <% @assignment = Assignment.find(node.node_object_id) %>
  <!--ACS Show the create team option in the assignment for the instructor only if
  the assignment allows teams to be formed with more than 1 participant-->
  <% if @assignment.max_team_size > 1 %>
  ...
</pre>
 
'''tree_display_controller.rb'''
<pre>child_nodes = root_node.get_children(sortvar,sortorder,session[:user].id,@show,nil,search)</pre>
 
=Conclusion=
In conclusion, we were able to successfully refactor the tree_display_controller and related controllers and views. We improved and expanded the use of RESTful and DRY design choices, implemented the use of routing helpers, ensured that instantiation of instance variables is minimized, and overall improved the quality and efficiency of the Expertiza codebase.  We also discovered a number of potential issues and additional areas for improvement.

Latest revision as of 06:22, 6 November 2014

E1463: Refactoring TreeDisplayController

Project Links

Our Expertiza Fork on AWS
Github Repo
Expertize Pull Request
Project Running on AWS

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.

Example Image Of Tree Display:

Refactoring TreeDisplayController Tasks

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

  1. Changing List to Index using a RESTful approach.
  2. Combined all of the goto methods
  3. Use of routing helpers
  4. Ensuring that instantiation of instance variables is minimized.

In the end, we enhanced this list a bit and resulted in doing the following:

  1. Changed `list` method to `index`
  2. Removed all of the `goto` methods and `drill`, and had them go through `index` instead because they are essentially just filtered versions of a listing (and the previous implementation just redirected each `goto` back to `list` anyway)
  3. Used path helpers where possible
  4. Removed instance variables and used local variables instead. This should be better for scope reasons. Note that there are still 1-2 instance variables used in the view, `@show` and `@assignment` which don't appear to be used anywhere. (`@show` doesn't seem to ever be set, `@assignment` seems to only be used locally) However, we didn't touch this because it didn't deal directly with our changes to the controller.
  5. Introduced some local variables as params to the view which allow dropdowns to keep their selected option after the page reloads (the logic was there before, but it was using instance variables which were never set!). This is for the search/filtering options on the "Manage..." screen.

1) Changing List to Index

In order to make the codebase more RESTful, we have replaced all the controller action redirections with a tree_display_index_path which enormously reduces the code content and as well as provides an organized approach to action redirections.

As seen below, we were able to remove list from Routes.rb:

resources :tree_display do 
collection do 
  get ':action' 
    - post 'list' 
  end 
end 

2) Combined All of the Goto Methods

We also combined all of the goto methods into one, in accordance with the DRY principle. This removed a significant amount of duplicate code.

Shown below are 3 of the 10 very similar methods we removed from tree_display_controller.rb:

- # direct access to questionnaires 
- 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 

These methods were replaced by a variable containing a list mapping all the session variable strings to database labels. The goto_assignments method was replaced with an index method to accept this variable, rather then calling each goto method individually.

Added variable:

+ @@Groups = { 
+   'Questionnaires' => 'Questionnaires', 
+   'Review rubrics' => 'Review', 
+   'Metareview rubrics' => 'Metareview', 
+   'Teammate review rubrics' => 'Teammate Review', 
+   'Author feedbacks' => 'Author Feedback', 
+   'Global survey' => 'Global Survey', 
+   'Surveys' => 'Survey', 
+   'Course evaluations' => 'Course Evaluation', 
+   'Courses' => 'Courses', 
+   'Bookmarkrating' => 'Bookmarkrating', 
+   'Assignments' => 'Assignments' 
+ } 

Removed goto_assignments method:

- # direct access to assignments 
- def goto_assignments 
-   node_object = TreeFolder.find_by_name('Assignments') 
-   session[:root] = FolderNode.find_by_node_object_id(node_object.id).id 
-   redirect_to :controller => 'tree_display', :action => 'list' 
- end 

Added index method:

+ def index 
+   session[:root] = params[:root] 
+   group = getGroup session[:menu] 
+   node_object = TreeFolder.find_by_name(group) 
+   if not group.blank? and not node_object.blank? 
+     session[:root] = FolderNode.find_by_node_object_id(node_object.id).id 
+ end 

3) Use Routing Helpers

We refactored the tree display controller class to use route helpers rather than calling ":action => 'list', :controller => 'tree_display'", or something similar. This involved many edits in the tree_display_controller.rb file and related controller and view files. As seen below, we utilized routing helpers and replaced ':controller=>'course', :action=>'new', :private => 0 with new_course_path(private: 0).

_courses_folder_actions.html.erb changes:

<ul> 
   <div> 
   <%= link_to image_tag('/assets/tree_view/add-public-24.png'), 
  - { :controller=>'course', :action=>'new', :private => 0 }, 
  - { :title => 'Create Public Course', id: 'create-public-course' } %> 
  + new_course_path(private: 0), 
  + { title: 'Create Public Course', id: 'create-public-course' } %> 
   <%= link_to image_tag('/assets/tree_view/add-private-24.png'), 
  - { :controller=>'course', :action=>'new', :private => 1 }, 
  - { :title => 'Create Private Course', id: 'create-private-course' } %> 
  + new_course_path(private: 1), 
  + { title: 'Create Private Course', id: 'create-private-course' } %> 
   </div> 
   </ul> 
   </li>
</ul> 

Another example of routing helpers in _bread_crumbs.html.erb.

Before:

<%= link_to curr_node.get_name, :controller => 'tree_display', :action => 'drill', :root => curr_node.id %>

After:

<%= link_to curr_node.get_name, tree_display_index_path(root: curr_node.id)%>

4) Minimizing the count of Instance Variables

Instance Variables in Ruby

An instance variable has a name beginning with @, and its scope is confined to whatever object self refers to. Two different objects, even if they belong to the same class, are allowed to have different values for their instance variables. From outside the object, instance variables cannot be altered or even observed (i.e., ruby's instance variables are never public) except by whatever methods are explicitly provided by the programmer. As with globals, instance variables have the nil value until they are initialized.

Instance variables do not need to be declared. This indicates a flexible object structure; in fact, each instance variable is dynamically appended to an object when it is first assigned.

We have observed that in Expertiza, there were several code files related to the Tree Display controller which were observed to be using multiple instance variables which could be reduced. Our team aimed to limit the instantiation of such variables to only necessary places in our code.


The following was added to the index function of tree_display_controller.rb in order to replace the use of instance variables with local variables:

locals  search:       search,
        sortvar:      sortvar,
        sortorder:    sortorder,
        root_node:    root_node,
        child_nodes:  child_nodes,
        filternode:   params[:filternode],
        searchnode:   params[:searchnode]

Where the locals method is defined as follows:

private

  # render local variable hash for view
  # reference: http://thepugautomatic.com/2013/05/locals/
  def locals(values)
    render locals: values
  end

Many of the view files were changed to make use of the local variables.

Here is an example of how _bread_crumbs.html.erb was changed. Note the removal of the "@" characters.

Before:

<% if @root_node %>
   <%= link_to "Return to top", tree_display_index_path %>
   <%
      curr_node = @root_node
   while curr_node.parent_id
     curr_node = Node.find(curr_node.parent_id) %>
     
   <%= link_to curr_node.get_name, tree_display_index_path(root: curr_node.id)%>
 <% end %>
  <%= @root_node.get_name %><%
end %>

After:

<% if root_node %>
   <%= link_to "Return to top", tree_display_index_path %>
   <%
      curr_node = root_node
   while curr_node.parent_id
     curr_node = Node.find(curr_node.parent_id) %>
     
   <%= link_to curr_node.get_name, tree_display_index_path(root: curr_node.id)%>
 <% end %>
  <%= root_node.get_name %><%
 end %>

Unexpected Database Changes

Unfortunately, each method of the tree_display controller has a reference to it stored in the database, along with a route. Essentially, the routes are being stored in the database and a wildcard is used in the routes table to allow this to happen. The following is a sample of some of the changes made (as part of tasks 1 and 2 given above) which were required in the db/seeds.rb file:

-ControllerAction.create(:site_controller_id => SiteController.find_by_name('tree_display').id, :name => 'goto_teammate_reviews', :permission_id => nil, :url_to_use => '')
-ControllerAction.create(:site_controller_id => SiteController.find_by_name('tree_display').id, :name => 'goto_metareview_rubrics', :permission_id => nil, :url_to_use => '')
-ControllerAction.create(:site_controller_id => SiteController.find_by_name('tree_display').id, :name => 'goto_teammatereview_rubrics', :permission_id => nil, :url_to_use => '')
+ControllerAction.create(:site_controller_id => SiteController.find_by_name('tree_display').id, :name => 'index', :permission_id => nil, :url_to_use => '')

In addition, these name attributes are used in other tables, such as the menu_items table. As a result, for every method in the tree_display_controller, changes like the following had to be made:

 MenuItem.create(:parent_id => MenuItem.find_by_name('manage/questionnaires').id, :name => 'manage/questionnaires/course evaluations', :label => 'Course evaluations', :seq => 7, :content_page_id => nil,
-                :controller_action_id => ControllerAction.where(site_controller_id: SiteController.find_by_name('tree_display').id, name:  'goto_surveys').first.id)
+                :controller_action_id => ControllerAction.where(site_controller_id: SiteController.find_by_name('tree_display').id, name:  'index').first.id)


Instead, we believe it would be better for this to be defined in the routes file. Having routes defined in the database is not transparent at all, and causes much confusion for developers that are new to the project - especially those which are new to Rails as well. This should be inspected for possible future refactorings.


Future Considerations

As mentioned in the "Unexpected Database Changes" section, routing should be removed from the database and the routes.rb file should be used instead. There are several other controllers/actions which are treated the same way.

In the views for tree_display, there are two instances variables which look like they could be removed - @show and @assignment. @show appears to never be set anywhere, despite it being used in tree_display_controller, _entry.html.erb, and _listing.html.erb. Perhaps it was previously used, but then it became obsolete and was never removed. @assignment is set inside of the view it is being used, actions/_assignments_actions.html.erb, but it appears to never be used elsewhere. In this case, @assignment should probably be replaced with a local variable. However, we did not touch on either of these issues because they were outside the scope of our task (@show is never set inside the controller and @assignment doesn't appear in the controller whatsoever). We focused primarily on only refactoring the controller and places in the view which were affected. In some cases, we went above and beyond and refactored additional parts of the tree_display views which were not required. However, we had to draw a line at some point due to time constraints.

_listing.html.erb

<% child_nodes = node.get_children(sortvar,sortorder,@show,nil,nil,search) %>

_entry.html.erb

<% child_nodes = node.get_children(sortvar,sortorder,session[:user].id,@show,search) %>

actions/_assignments_actions.html.erb

  <% @assignment = Assignment.find(node.node_object_id) %>
  <!--ACS Show the create team option in the assignment for the instructor only if
  the assignment allows teams to be formed with more than 1 participant-->
  <% if @assignment.max_team_size > 1 %>
  ...

tree_display_controller.rb

child_nodes = root_node.get_children(sortvar,sortorder,session[:user].id,@show,nil,search)

Conclusion

In conclusion, we were able to successfully refactor the tree_display_controller and related controllers and views. We improved and expanded the use of RESTful and DRY design choices, implemented the use of routing helpers, ensured that instantiation of instance variables is minimized, and overall improved the quality and efficiency of the Expertiza codebase. We also discovered a number of potential issues and additional areas for improvement.