CSC/ECE 517 Fall 2017/E1772 Refactor reputation web service controller.rb

Introduction

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.

Problem Statement

Online peer-review systems are now in common use in higher education. They free the instructor and course staff from having to provide personally all the feedback that students receive on their work. However, if we want to assure that all students receive competent feedback, or even use peer assigned grades, we need a way to judge which peer reviewers are most credible. The solution is the reputation system. Reputation systems have been deployed as web services, peer-review researchers will be able to use them to calculate scores on assignments, both past and present (past data can be used to tune the algorithms).

For this project, our team's job is to refactor the file: reputation_web_service_controller.rb. This file is the controller to calculate the reputation scores. A “reputation” measures how close a reviewer’s scores are to other reviewers scores. This controller is the sub-class of Application_Controller, in this controller, it implements the calculation and query of reputation scores.

---

Issues to be fixed

Reputation_web_service_controller.rb is a fairly complex file. It contains a lot of methods that are long and hard to understand. These methods need to be broken down into simpler and more specific methods that are easier to read/understand. Also, the few instances of code duplication that exist should be removed.

1. Replace class variable with a class instance var, and change all other places using these variables.
2. Refactor method db_query
3. Use zero? method instead of == 0
4. Use find_by instead of dynamic method
5. Delete lines which is wrong or useless.
6. Comment lines

---

Modified Files

reputation_web_service_controller.rb

---

NewFiles

reputation_web_service_controller_spec.rb

---

Important links

Demo Video Link

Project Repo

Github Pull Request

---

Approach taken to resolve the issues

Replace class variable with a class instance var, and change all other places using these variables.

In ruby, class variables are started with '@@'and this class variable are shared in the whole inherit chain, and class variables are shared by all objects of a class. For instance variables, in Ruby, instance variables are started with '@' and instance variables belong to one object, can not be used by sub-classes.

In the project, we are asked to replace the class variable with a class instance variables. So, as the figure 1 shown, in variables declaration part, we changed all class variables to instance variables.

Next, we need to change all other places using these variables.

In action send_post_request

---

Refactor method db_query

In the Reputation_web_service controller, the action db_query is important, because it implements the query of peer review grade.To refactor the db_query method, we need to firstly focus on the input variables of this method, there are 4 variables need inputs. The db_query method is shown as follow:

  def db_query(assignment_id, another_assignment_id = 0, round_num, hasTopic)
      ...
  end

There is a problem: Optional arguments should appear at the end of the argument list, in this method, another_assignment_id is a optional arguments, but in the original version, the optional arguments -- another_assignment_id just appear right behind the argument--assignment_id, so we need to refactor it.

And the same problems occurs when we assigns value to variable @results. We also need to change it.

There is another problem in the db_query method, we can see there is a input variable named hasTopic, but actually that's not follow the "good Ruby and Rails coding practices", so we need to refactor it which shown as follow:

Finally, The project asked us 'Move Line 26-50 before this method', it's important for code refactor, because it's very inelegant if you put the comments inside the method. If others want to read your codes, it will cost their much more time so that it needs to be changed. Simply, we can just move the whole comments before the method, and that will make the codes easily for reading.

Use zero? method instead of == 0.

In the original version of project, every time there is a zero judgement occurs, it use '==0' or '!=0', of course it can achieve the judgement of zero, but according to the security consideration in ruby on rails development, we need to avoid the use of '==0' or '!=0'. So the best way to solve it is using the zero? instead of '==0' or '!=0', which shown as follow:

Use find_by instead of dynamic method.

This issue is replacing the find_by_id to find_by(id: id). The only difference of this change is syntax and their function worked in the same way. But if the customer want to change the way to find assignment, find_by(id: id) would be easier to change because programmer only need to change the parameter, while find_by_id need to change the whole method and its parameter. Therefore, in this project, using find_by_id is more efficient.

Delete lines which is wrong or useless.

In Line 111, the variable inner_msg has never been used, so it can be deleted

From line 164 to 201 and line 204 to 241, cases for reputation value and do not need to handle the case for assign id. req body would change only in the case for 'Add expert grades'. If these two part failed to be deleted, the body would append three times and output wrong result.

Comment lines.

The function for controller is not for output but just control and update the change of view. So puts should not appear in controller

RSpec Test

Our test plan is to test if db_query(assignment_id, round_num, has_topic, another_assignment_id = 0)(in line 51), db_query_with_quiz_score(assignment_id, another_assignment_id = 0)(int line 82), json_generator(assignment_id, another_assignment_id = 0, round_num = 2, type = 'peer review grades')(in line 101) work in the right way. These are in controller/reputation_web_service_controller.rb

require 'rspec'

describe ReputationWebServiceController do

  before(:each) do
    user = build(:instructor)
    stub_current_user(user, user.role.name, user.role)
  end

  it 'should do execute query' do
    #controller.json_generator(41,0,2,'peer review grades').should != nil
    has_topic = !SignUpTopic.where(41).empty?
    raw_data_array = []
    raw_data_array = controller.db_query(41,2,has_topic,0)
    expect(controller.db_query(41,2,has_topic,0)).to be_an_instance_of(Array)
    expect(controller.db_query(41,2,has_topic,0)).should_not be(nil)
    expect(raw_data_array).should_not be(nil)
  end

The above codes is to test if get query can parse id and some other variables and return the correct review grade. Our test make sure the review grade are array and array contains data which is not null.

  it 'should execute query for quiz score' do
    result = controller.db_query_with_quiz_score(55,0)
    expect(result).to be_an_instance_of(Array)
    expect(result).should_not be(nil)
    #puts result
  end

The above code is to test if we can query quiz score by given assignment id and another assignment id and return correct quiz score array. Our test make sure that the the result is array and not a null array

But when we try to test json_generator(assignment_id, another_assignment_id = 0, round_num = 2, type = 'peer review grades'). There is a wrong report which is no method. the line 106 that is no method is: @results = db_query(assignment.id, round_num, has_topic, another_assignment_id) And the report is that:

Because this method can't be called for some reason and this is not the part that we need to refactor, we can't test this part.