E1570. Refactoring TreeDisplayController

This page provides a description of the Expertiza based OSS project. This project aimed at refactoring the TreeDisplayController.

Introduction to Expertiza

Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). The Expertiza project is supported by the National Science Foundation.<ref>Expertiza</ref><ref>Wiki</ref>

Problem Statement

Classes involved:

tree_display_controller.rb

What they do: This controller deals with displaying “trees” of objects to the instructor. When an instructor logs in, (s)he is greeted with a homepage that lists questionnaires (rubrics, quizzes, surveys), courses, and assignments. These objects are displayed in a “tree,” allowing the user to click on the top-level object and see the objects beneath it.

What's wrong with it:

Many Duplicate Methods.
Very Long Methods combining many Functionalities.
Redundant methods not being used anywhere.

What needs to be done:

Split get_children_node_ng and get_children_node_2_ng into smaller methods and give reasonable names to them and make sure implement common code in a single method.

Changes Made

TreeDisplayController

Method Name	Changes Made	Reason For Change
goto_questionnaires, goto_review_rubrics, goto_metareview_rubrics, goto_teammatereview_rubrics, goto_author_feedbacks, goto_global_survey, goto_surveys, goto_course_evaluations, goto_courses, goto_assignments	All these methods were merged into a single method with name go_to_menu_items which receives a parameter of the name of tree_folder object, such as "Questionnaires".	Since all the methods were performing the same action, i.e passing the node object id to list method in TreeDisplayController, the name is passed a parameter to the go_to_menu_items method from "link" method in MenuItemsController. Using this parameter the respective object and its ID are found and is redirected to "list" method.
get_children_node_ng	Moved part of the code from these methods to newly created methods populate_rows,display_row,page_render methods.	These Methods had many functionalities being implemented which made the method look lengthy and moreover these methods were performing almost similar functions which could be grouped together and reduce the overall code length.
get_children_node_2_ng
Put ur method here	Put ur txt here	put ur text here

MenuItemsController

Method Name	Changes Made	Reason For Change
link	Redirect url only for the menu_items altered in TreeDisplayController is changed to go_to_menu_items_url.	Since the 10 methods had been merged into one method in TreeDisplayController, the redirect url is set to go_to_menu_items url.

Re-factored Code

Refactoring get_children_node_ng method

Before Changes

After Changes

  def get_children_node_ng
    childNodes = {}
    if params[:reactParams][:child_nodes].is_a? String
      childNodes = JSON.parse(params[:reactParams][:child_nodes])
    else
      childNodes = params[:reactParams][:child_nodes]
    end
    tmpRes = {}
    res = {}
    for node in childNodes
      fnode = eval(params[:reactParams][:nodeType]).new

      for a in node
        fnode[a[0]] = a[1]
      end

      # fnode is the parent node
      # ch_nodes are childrens
      ch_nodes = fnode.get_children(nil, nil, session[:user].id, nil, nil)
      tmpRes[fnode.get_name] = ch_nodes

      # cnode = fnode.get_children("created_at", "desc", 2, nil, nil)

    end

    for nodeType in tmpRes.keys
      res[nodeType] =  Array.new

      for node in tmpRes[nodeType]
        tmpObject = {}
        tmpObject["nodeinfo"] = node
        tmpObject["name"] = node.get_name
        tmpObject["type"] = node.type

        if nodeType == 'Courses' || nodeType == "Assignments"
          tmpObject["directory"] = node.get_directory
          tmpObject["creation_date"] = node.get_creation_date
          tmpObject["updated_date"] = node.get_modified_date
          tmpObject["private"] = node.get_private
          instructor_id = node.get_instructor_id
          tmpObject["instructor_id"] = instructor_id
          unless (instructor_id.nil?)
            tmpObject["instructor"] = User.find(instructor_id).name
          else
            tmpObject["instructor"] = nil
          end

          tmpObject["is_available"] = is_available(session[:user], instructor_id) || (session[:user].role_id == 6 && 

Ta.get_my_instructors(session[:user].id).include?(instructor_id) && ta_for_current_course?(node))
          if nodeType == "Assignments"
            tmpObject["course_id"] = node.get_course_id
            tmpObject["max_team_size"] = node.get_max_team_size
            tmpObject["is_intelligent"] = node.get_is_intelligent
            tmpObject["require_quiz"] = node.get_require_quiz
            tmpObject["allow_suggestions"] = node.get_allow_suggestions
            tmpObject["has_topic"] = SignUpTopic.where(['assignment_id = ?', node.node_object_id]).first ? true : false
          end
        end
        res[nodeType] << tmpObject
      end

    end

    respond_to do |format|
      format.html {render json: res}
    end
  end

   def get_children_node_ng
    childNodes = {}
    if params[:reactParams][:child_nodes].is_a? String
      childNodes = JSON.parse(params[:reactParams][:child_nodes])
    else
      childNodes = params[:reactParams][:child_nodes]
    end
    ch_nodes = {}
    for node in childNodes
      # Declaring Foldernode Object as New
      fnode = eval(params[:reactParams][:nodeType]).new
      for a in node
        fnode[a[0]] = a[1]
      end
      # fnode is the parent node
      # ch_nodes are childrens
      ch_nodes[fnode.get_name] = fnode.get_children(nil, nil, session[:user].id, nil, nil)
      # cnode = fnode.get_children("created_at", "desc", 2, nil, nil)
    end
      call_function ="get_children_node_ng"
      #Render JSON of the child nodes
      populate_rows(ch_nodes,call_function)
  end

Refactoring get_children_node_2_ng method

Before Changes

After Changes

  def get_children_node_2_ng
    childNodes = {}
    if params[:reactParams2][:child_nodes].is_a? String
      childNodes = JSON.parse(params[:reactParams2][:child_nodes])
    else
      childNodes = params[:reactParams2][:child_nodes]
    end
    tmpRes = {}
    res = []
    fnode = eval(params[:reactParams2][:nodeType]).new
    childNodes.each do |key, value|
      fnode[key] = value
    end

    ch_nodes = fnode.get_children(nil, nil, session[:user].id, nil, nil)
    tmpRes = ch_nodes
    if tmpRes
      for child in tmpRes
        nodeType = child.type
        res2 = {}
        res2["nodeinfo"] = child
        res2["name"] = child.get_name
        res2["key"] = params[:reactParams2][:key]
        res2["type"] = nodeType

        res2["private"] = child.get_private
        res2["creation_date"] = child.get_creation_date
        res2["updated_date"] = child.get_modified_date
        if nodeType == 'CourseNode' || nodeType == "AssignmentNode"
          res2["directory"] = child.get_directory
          instructor_id = child.get_instructor_id
          res2["instructor_id"] = instructor_id
          unless (instructor_id.nil?)
            res2["instructor"] = User.find(instructor_id).name
          else
            res2["instructor"] = nil
          end

          res2["is_available"] = is_available(session[:user], instructor_id) || (session[:user].role_id == 6 && 
Ta.get_my_instructors(session[:user].id).include?(instructor_id) && ta_for_current_course?(child))
          if nodeType == "AssignmentNode"
            res2["course_id"] = child.get_course_id
            res2["max_team_size"] = child.get_max_team_size
            res2["is_intelligent"] = child.get_is_intelligent
            res2["require_quiz"] = child.get_require_quiz
            res2["allow_suggestions"] = child.get_allow_suggestions
            res2["has_topic"] = SignUpTopic.where(['assignment_id = ?', child.node_object_id]).first ? true : false
          end
        end
        res << res2
      end
    end

    respond_to do |format|
      format.html {render json: res}
    end
  end

 def get_children_node_2_ng
    # Second Level of Tree Display with Children Nodes
    childNodes = {}
    if params[:reactParams2][:child_nodes].is_a? String
      childNodes = JSON.parse(params[:reactParams2][:child_nodes])
    else
      childNodes = params[:reactParams2][:child_nodes]
    end

    res = []
    fnode = eval(params[:reactParams2][:nodeType]).new
    childNodes.each do |key, value|
      fnode[key] = value
    end

    ch_nodes = fnode.get_children(nil, nil, session[:user].id, nil, nil)

    call_function = "get_children_node_2_ng"
    populate_rows(ch_nodes, call_function)
  end

Common Methods Added to get_children_node_ng and get_children_node_2_ng controllers

populate_rows

This is a new Common Method for both get_children_node_ng and get_children_node_2_ng that does the functionality of rendering a page with data from all nodes by calling another method populate_1_row(). There is another Method call that happens in this method which as a whole renders the List View.

populate_rows()

  def populate_rows(list,call_function) #render page with data for all nodes in list
    if call_function == "get_children_node_ng"
      tmpRes ={}
      tmpRes = list
      res = {}
       for nodeType in tmpRes.keys
        # declaring a new array
        res[nodeType] =  Array.new
        for node in tmpRes[nodeType]
          res[nodeType] << populate_1_row(node)
        end
       end

    else
      tmpRes = list
      res = []
      if tmpRes
        for child in tmpRes
          res2 = {}
          res2 = populate_1_row(child)
          res2["key"] = params[:reactParams2][:key]
          res << res2
        end
      end
    end
    page_render(res)
  end

}

populate_1_row

 This is a new Method that is called from Method populate_row() and fetches all the child node names from respective Controller actions and then populates the tmpObject.

populate_1_row()

  def populate_1_row(node) #return JSON for 1 Node
    tmpObject = {}
    tmpObject["nodeinfo"] = node
    # all the child nodes names got and put in tmpObject from respective controller actions
    tmpObject["name"] = node.get_name
    tmpObject["type"] = node.type
    if node.type == 'CourseNode' || node.type == "AssignmentNode"
      tmpObject["directory"] = node.get_directory
      tmpObject["creation_date"] = node.get_creation_date
      tmpObject["updated_date"] = node.get_modified_date
      tmpObject["private"] = node.get_private
      instructor_id = node.get_instructor_id
      tmpObject["instructor_id"] = instructor_id
      unless (instructor_id.nil?)
        tmpObject["instructor"] = User.find(instructor_id).name
      else
        tmpObject["instructor"] = nil
      end

      tmpObject["is_available"] = is_available(session[:user], instructor_id) || (session[:user].role_id == 6 && Ta.get_my_instructors(session[:user].id).include?(instructor_id) && ta_for_current_course?(node))
      if node.type == "AssignmentNode"
        tmpObject["course_id"] = node.get_course_id
        tmpObject["max_team_size"] = node.get_max_team_size
        tmpObject["is_intelligent"] = node.get_is_intelligent
        tmpObject["require_quiz"] = node.get_require_quiz
        tmpObject["allow_suggestions"] = node.get_allow_suggestions
        tmpObject["has_topic"] = SignUpTopic.where(['assignment_id = ?', node.node_object_id]).first ? true : false
      end
    end
    tmpObject
  end

}

page_render

 This is also a common Method which integrates the rendering functionality of get_children_node_ng and get_children_node_2_ng making sure both levels are properly populated in the view.

page_render()

  def page_render(list) # Common Render Functionality for get_children_node_ng and get_children_node_2_ng methods
    respond_to do |format|
      format.html {render json: list}
    end
  end

}

Method Removal

There was a Method which dint have any Routes defined in the Routes.rb file and was not called in any of the Controller . After Careful analysis we descided to remove the method from the tree Display Controller.

filter

  def filter
    search = params[:filter_string]
    filter_node = params[:filternode]
    qid = 'filter+'

    if filter_node == 'QAN'
      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.to_s}+" }
          session[:root] = 1
        end
      end
    elsif filter_node == 'ACN'
      session[:root] = 2
      qid <<  search
    end
    return qid
  end

}

Removing duplicate methods

Anush For u to write here...

Before Changes

After Changes

def new_feedback
    review = Response.find(params[:id])
    if review
      reviewer = AssignmentParticipant.where(user_id: session[:user].id, parent_id:  review.map.assignment.id).first
      map = FeedbackResponseMap.where(reviewed_object_id: review.id, reviewer_id:  reviewer.id).first
      if map.nil?
        map = FeedbackResponseMap.create(:reviewed_object_id => review.id, :reviewer_id => reviewer.id, 
                                         :reviewee_id => review.map.reviewer.id)
      end
      redirect_to :action => 'new', :id => map.map_id, :return => "feedback"
    else
      redirect_to :back
    end
  end

def new_feedback
    review = Response.find(params[:id])
    if review
      reviewer = AssignmentParticipant.where(user_id: session[:user].id, parent_id:  review.map.assignment.id).first
      map = FeedbackResponseMap.where(reviewed_object_id: review.id, reviewer_id:  reviewer.id).first
      if map.nil?
        #if no feedback exists by dat user den only create for dat particular response/review
        map = FeedbackResponseMap.create(:reviewed_object_id => review.id, :reviewer_id => reviewer.id, 
                                         :reviewee_id => review.map.reviewer.id)
      end
      redirect_to :action => 'new', :id => map.id, :return => "feedback"
    else
      redirect_to :back
    end
  end

def new_feedback
    review = Response.find(params[:id])
    if review
      reviewer = AssignmentParticipant.where(user_id: session[:user].id, parent_id:  review.map.assignment.id).first
      map = FeedbackResponseMap.where(reviewed_object_id: review.id, reviewer_id:  reviewer.id).first
      if map.nil?
        #if no feedback exists by dat user den only create for dat particular response/review
        map = FeedbackResponseMap.create(:reviewed_object_id => review.id, :reviewer_id => reviewer.id, 
                                         :reviewee_id => review.map.reviewer.id)
      end
      redirect_to :action => 'new', :id => map.id, :return => "feedback"
    else
      redirect_to :back
    end
  end

def view
    @response = Response.find(params[:id])
    return if redirect_when_disallowed(@response)
    @map = @response.map
    get_content
    @review_scores = Array.new
    @question_type = Array.new
    @questions.each do |question|
      @review_scores << Score.where(response_id: @map.response_id, question_id:  question.id).first
      @question_type << QuestionType.find_by_question_id(question.id)
    end
  end

def view
    @response = Response.find(params[:id])
    return if action_allowed?(@response)
    @map = @response.map
    get_content
    get_scores(@response, @questions)
  end

def view
    @response = Response.find(params[:id])
    return if action_allowed?(@response)
    @map = @response.map
    get_content
    get_scores(@response, @questions)
  end

Rspec Test

 We have written Rspec tests for the tree_display_controller and have run the same with success.

page_render()

require 'rails_helper'

describe TreeDisplayController do

  describe "#list" do
    it "should not redirect to student_task controller if current user is an instructor" do
      allow(session[:user]).to receive("student?").and_return(false)
      post "list"
      response.should_not redirect_to(list_student_task_index_path)
    end
    it "should redirect to student_task controller if current user is a student" do
      allow(session[:user]).to receive("student?").and_return(true)
       post "list"
       response.should redirect_to(list_student_task_index_path)
    end
  end

  describe "#ta_for_current_mappings?" do
    it "should return true if current user is a TA for current course" do
        allow(session[:user]).to receive("ta?").and_return(true)
    end
  end

  describe "#populate_rows" do
    let(:dbl) { double }
    before { expect(dbl).to receive(:populate_rows).with(Hash, String)}
    it "passes when the arguments match" do
      dbl.populate_rows({},"")
    end
  end

  describe "#populate_1_row" do
    let(:dbl) { double }
    before { expect(dbl).to receive(:populate_1_row).with(Node) }
    it "passes when the arguments match" do
      dbl.populate_1_row(Node.new)
    end
  end
end

}

References

CSC/ECE 517 Fall 2015/oss E1570 avr

Contents

Introduction to Expertiza

Problem Statement

Changes Made

TreeDisplayController

MenuItemsController

Re-factored Code

Refactoring get_children_node_ng method

Refactoring get_children_node_2_ng method

Common Methods Added to get_children_node_ng and get_children_node_2_ng controllers

populate_rows

populate_1_row

page_render

Method Removal

filter

Removing duplicate methods

Rspec Test

References

Navigation menu

CSC/ECE 517 Fall 2015/oss E1570 avr

Introduction to Expertiza

Problem Statement

Changes Made

TreeDisplayController

MenuItemsController

Re-factored Code

Refactoring get_children_node_ng method

Refactoring get_children_node_2_ng method

Common Methods Added to get_children_node_ng and get_children_node_2_ng controllers

populate_rows

populate_1_row

page_render

Method Removal

filter

Removing duplicate methods

Rspec Test

References

Navigation menu

Search