CSC/ECE 517 Fall 2020 - E2064. Refactor reputation web service controller.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
(Created page with "=='''Introduction'''== ==='''Introduction to Expertiza'''=== [http://expertiza.ncsu.edu/ Expertiza] is a peer review based system which provides incremental learning from the...")
 
No edit summary
 
(102 intermediate revisions by 3 users not shown)
Line 3: Line 3:
[http://expertiza.ncsu.edu/ 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 [http://rubyonrails.org/ 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.  
[http://expertiza.ncsu.edu/ 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 [http://rubyonrails.org/ 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.
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'''===
==='''About Reputation Web Service Controller'''===
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).  
Expertiza allows student work to be peer-reviewed, since peers can provide more feedback than the instructor can. 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 implements the calculation of reputation scores.
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'''==
=='''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.
reputation_web_service_controller.rb is a fairly complex file. It contains a lot of methods that are long and hard to understand (for e.g. send_post_request). 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.
 
# There is a lot of unused/commented code, which should be removed.
# Figure out what the code is doing and write appropriate comments for it.
# Rename bad method names such as (db_query, json_generator). Method names should be verbs, and should say what the method does.  Method names should not be so general that they could apply to many different methods..
# In the case of db_query, the name should say what it queries for. Also, this method not only queries, but calculates sums.  Since each method should do only one thing, the code for calculating sums should be in another method. And there should be comments in the code!
#json_generator should be generate_json. There needs to be a method comment saying what the parameters are.
# In  send_post_request, there are references to specific assignments, such as 724, 735, and 756. They were put in to gather data for a paper published in 2015. They are no longer relevant and should be removed. send_post_request is 91 lines long, far too long.
#There is a password for a private key in the code (and the code is open-sourced!) It should be in the db instead.
#Fix spelling of “dimention”
#client is a bad method name; why is stuff being copied from class variables to instance variables?
 


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


=='''Steps taken to resolve the issues'''==


----
----


=='''Modified Files'''==
=== Refactor db_query===
 
  Renaming of bad method names such as (<code>db_query</code>). Method names should be verbs, and should say what the method does.
Method names should not be so general that they could apply to many different methods.
The other problem with the method was that it did not follow the good practice of having each method perform only one task.
This method got the review responses with a db query.
Also, it iterated over the review responses to perform a calculation of peer review grades.
 
We have split the method into two to have the review responses do only the get responses part.
 
[[File:snip7.png]]
 
 
<br>
 
=== The code for calculating sums should be in another method. ===
  The result of this method is stored and we have created a new method <code>calculate_peer_review_grades</code> and moved the calculation part to it. <br>
We also added a comment to indicate the calculate weighted sum part.
 
<br>
[[File:snip8.png]]
 
<br><br>
  And then, we have changed the call to the <code> get review responses</code> and the new method calculate peer review grades accordingly
<br><br>
[[File:snip3.png]]
<br><br>
 
Similarly, we have changed <code>db_query_with_quiz_scores</code> to <code>calculate_quiz_scores</code>
 
<br>
[[File:snip5.png]]
<br><br>
 
===''' Change name of json_generator'''===
 
  json_generator method should be <code>generate_json</code>
Also,there needs to be a method comment saying what the parameters are.
We have added the method comments with parameter information as seen below.
 
[[File:snip4.png]]
 
===''' Refactor send_post_request'''===
 
“In (<code>send_post_request</code>), there are references to specific assignments, such as 724, 735, and 756.
  They were put in to gather data for a paper published in 2015. They are no longer relevant and should be removed.
  send_post_request is 91 lines long, far too long”
 
We observed that encryption of request being sent to evaluate Algorithm and decryption of response is written in (<code>send_post_request</code>) only that is why send post is 91 lines long.
We have created two separate methods
(<code>encrypt_request</code>)
(<code>decrypt_response</code>)
 
In the image below we can see that we have separated Encryption functionality from (<code>send_post_request</code>) to (<code>encrypt_request</code>)
 
[[File:encryption-separated.jpg]]


reputation_web_service_controller.rb
On the similar lines we have separated Decryption functionality from (<code>send_post_request</code>) to (<code>decrypt_response</code>)


[[File:decryption-separated.jpg]]


(<code>encrypt_request</code>) has been refactored as follows


----
[[File:encrypt_request.png]]


=='''NewFiles'''==
(<code>decrypt_response</code>) has been refactored as follows


reputation_web_service_controller_spec.rb
[[File:decrypt_request.png]]


===''' Fixed spelling of dimention'''===


----
[[File:capture2.png]]
=='''Important links'''==
[https://www.youtube.com/watch?v=jdL-L-UZp1A&t=20s Demo Video Link]<br>


[https://github.com/yuxuyang/expertiza Project Repo]<br>
==='''Updating client method'''===


[https://github.com/expertiza/expertiza/pull/1027 Github Pull Request]


----
We changed the class variables in the code to instance variables because we realized that the send_post_request method and client method were the only methods using the class variables.
=='''Approach taken to resolve the issues'''==
A client.html.erb file was used to call our controller to fill out a form for send_post_request which set values of class variables then immediately redirected control to the client method which set the instance variables to those class variables. The instance variables were then used in the rest of the client.html.erb file.  
==='''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.


[[File:Reputation code1.png]]
The process of setting class variables to instance variables with a function was redundant, so we removed simply set the instance variables within the send_post_request method and took them out of the client method.
The client method did have a few actions that it completed besides setting class variables which we have left in the method until we can find a solution to replacing them.


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


In action '''send_post_request'''
[[File:old-client-method.png|frame|none|alt=Alt text|Old client method]]


[[File:Reputaion 2.png]]


[[File:Reputaion3.png]]
[[File:new-client-method-and-helpers-new.png|frame|none|alt=Alt text|Newest client method]]


[[File:Reputation4.png]]


[[File:Reputation5.png]]
The reason why we kept the client method's name is because the routes.rb file specifies the client method in the code and we haven't fully tracked down the impact of that yet.


[[File:Reputation6.png]]


----
== Testing ==
=== Test Plan ===
There was no existing tests available for the Reputation web service controller.
Since we are refactoring it, Our testing plan is to have an automated testing to ensure that the refactoring such as splitting a method into separate methods does not affect the existing implementation.
We also added steps for the manual testing to make sure the page loads as expected after the refactoring.
<br>
For the automated testing, We are using Rspec tests.
<br>
===RSpec===
We have tested the two new methods created during refactoring and also a renamed method.
The test coverage has increased a bit by 0.6% to 44%.
<br>


==='''Refactor method db_query'''===
The tests we created can also be manually run with command <br>
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.
<code>rspec spec/controllers/reputation_web_controller_spec.rb. </code>
The db_query method is shown as follow:
<br>
<pre>
  def db_query(assignment_id, another_assignment_id = 0, round_num, hasTopic)
      ...
  end
</pre>
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.


[[File:Reputation8.png]]
[[File:snip13.png]]


And the same problems occurs when we assigns value to variable '''''@results'''''. We also need to change it.
===Manual UI Testing===
<br>
The steps to test our branch are below, but they are failing right now due to errors that are in other files of the beta branch.


[[File:Reputation10.png]]


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:
To test our program you must login to the site here: http://152.7.98.78:8080/


[[File:Reputation9.png]]
Username: instructor6
Password: password


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.


[[File:Reputation11.png]]
Then the page "Manage content" doesn't show up, go to Manage... -> Assignments


==='''Use zero? method instead of == 0.'''===
[[File:snip14.png]]


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:
Then on the CSC 517, Fall 2017 Row, click on "Add Participants" in the actions column. This is also indicated by a blue shirted person.
[[File:Reputation12.png]]


[[File:Reputation13.png]]
[[File:snip15.png]]


==='''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.
[[File:refactor15.png]]
[[File:refactor16.png]]


==='''Delete lines which is wrong or useless.'''===
Once in there, copy on of the student names (such as student7487) and then go to Manage... -> Impersonate User and enter their name (student7487) into the text field.
In Line 111, the variable inner_msg has never been used, so it can be deleted
[[File:refactor17.png]]


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.
[[File:snip16.png]]
[[File:refactor21.png]]




[[File:refactor19.png]]
After impersonating them, you should be able to see their assignments. Click on any of their assignments such as "Program 1", then click on "Alternate View" to the right of "Your Scores".


==='''Comment lines.'''===
[[File:snip17.png]]
The function for controller is not for output but just control and update the change of view. So puts should not appear in controller


[[File:refactor22.png]]
[[File:snip18.png]]
[[File:refactor23.png]]


==='''RSpec Test'''===
<br>
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
----
<pre>
require 'rspec'


describe ReputationWebServiceController do
=='''Modified Files'''==
<code>reputation_web_service_controller.rb</code>


  before(:each) do
=='''New Files'''==
    user = build(:instructor)
<code>reputation_web_service_controller_spec.rb</code>
    stub_current_user(user, user.role.name, user.role)
  end


  it 'should do execute query' do
==Team Information==
    #controller.json_generator(41,0,2,'peer review grades').should != nil
'''Project Mentor:'''<br>
    has_topic = !SignUpTopic.where(41).empty?
Saurabh Shingte
    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
</pre>
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.


<pre>
'''Project Members:'''<br>
  it 'should execute query for quiz score' do
Abhishek Gupta <br>
    result = controller.db_query_with_quiz_score(55,0)
Skieler Capezza<br>
    expect(result).to be_an_instance_of(Array)
Ummu Kolusum Yasmin Ahamed Adam
    expect(result).should_not be(nil)
    #puts result
  end
</pre>
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:
=='''References'''==
@results = db_query(assignment.id, round_num, has_topic, another_assignment_id)
And the report is that:
[[File:24.jpeg]]


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.
*[https://github.com/srcapezza/expertiza/tree/beta Project Repo]<br>
*[https://github.com/expertiza/expertiza/pull/1790 Github Pull Request]
*[https://github.com/expertiza/expertiza Expertiza on GitHub]
*[http://expertiza.ncsu.edu/ The live Expertiza website]
*[http://research.csc.ncsu.edu/efg/expertiza Expertiza project Details]
*[https://www.youtube.com/channel/UCdKXzox7hrWjfOMML6FzTWg Expertiza YouTube Channel]

Latest revision as of 03:31, 20 October 2020

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.

About Reputation Web Service Controller

Expertiza allows student work to be peer-reviewed, since peers can provide more feedback than the instructor can. 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 implements the calculation 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 (for e.g. send_post_request). 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. There is a lot of unused/commented code, which should be removed.
  2. Figure out what the code is doing and write appropriate comments for it.
  3. Rename bad method names such as (db_query, json_generator). Method names should be verbs, and should say what the method does. Method names should not be so general that they could apply to many different methods..
  4. In the case of db_query, the name should say what it queries for. Also, this method not only queries, but calculates sums. Since each method should do only one thing, the code for calculating sums should be in another method. And there should be comments in the code!
  5. json_generator should be generate_json. There needs to be a method comment saying what the parameters are.
  6. In send_post_request, there are references to specific assignments, such as 724, 735, and 756. They were put in to gather data for a paper published in 2015. They are no longer relevant and should be removed. send_post_request is 91 lines long, far too long.
  7. There is a password for a private key in the code (and the code is open-sourced!) It should be in the db instead.
  8. Fix spelling of “dimention”
  9. client is a bad method name; why is stuff being copied from class variables to instance variables?


Steps taken to resolve the issues

Refactor db_query

 Renaming of bad method names such as (db_query). Method names should be verbs, and should say what the method does.

Method names should not be so general that they could apply to many different methods. The other problem with the method was that it did not follow the good practice of having each method perform only one task. This method got the review responses with a db query. Also, it iterated over the review responses to perform a calculation of peer review grades.

We have split the method into two to have the review responses do only the get responses part.



The code for calculating sums should be in another method.

 The result of this method is stored and we have created a new method calculate_peer_review_grades and moved the calculation part to it.

We also added a comment to indicate the calculate weighted sum part. 




 And then, we have changed the call to the  get review responses and the new method calculate peer review grades accordingly





Similarly, we have changed db_query_with_quiz_scores to calculate_quiz_scores




Change name of json_generator

 json_generator method should be generate_json

Also,there needs to be a method comment saying what the parameters are. We have added the method comments with parameter information as seen below.

Refactor send_post_request

“In (send_post_request), there are references to specific assignments, such as 724, 735, and 756. 
 They were put in to gather data for a paper published in 2015. They are no longer relevant and should be removed.
 send_post_request is 91 lines long, far too long”

We observed that encryption of request being sent to evaluate Algorithm and decryption of response is written in (send_post_request) only that is why send post is 91 lines long. We have created two separate methods (encrypt_request) (decrypt_response)

In the image below we can see that we have separated Encryption functionality from (send_post_request) to (encrypt_request)

On the similar lines we have separated Decryption functionality from (send_post_request) to (decrypt_response)

(encrypt_request) has been refactored as follows

(decrypt_response) has been refactored as follows

Fixed spelling of dimention

Updating client method

We changed the class variables in the code to instance variables because we realized that the send_post_request method and client method were the only methods using the class variables. A client.html.erb file was used to call our controller to fill out a form for send_post_request which set values of class variables then immediately redirected control to the client method which set the instance variables to those class variables. The instance variables were then used in the rest of the client.html.erb file.


The process of setting class variables to instance variables with a function was redundant, so we removed simply set the instance variables within the send_post_request method and took them out of the client method. The client method did have a few actions that it completed besides setting class variables which we have left in the method until we can find a solution to replacing them.


Alt text
Old client method


Alt text
Newest client method


The reason why we kept the client method's name is because the routes.rb file specifies the client method in the code and we haven't fully tracked down the impact of that yet.


Testing

Test Plan

There was no existing tests available for the Reputation web service controller. Since we are refactoring it, Our testing plan is to have an automated testing to ensure that the refactoring such as splitting a method into separate methods does not affect the existing implementation. We also added steps for the manual testing to make sure the page loads as expected after the refactoring.
For the automated testing, We are using Rspec tests.

RSpec

We have tested the two new methods created during refactoring and also a renamed method. The test coverage has increased a bit by 0.6% to 44%.

The tests we created can also be manually run with command
rspec spec/controllers/reputation_web_controller_spec.rb.

Manual UI Testing


The steps to test our branch are below, but they are failing right now due to errors that are in other files of the beta branch.


To test our program you must login to the site here: http://152.7.98.78:8080/

Username: instructor6 Password: password


Then the page "Manage content" doesn't show up, go to Manage... -> Assignments

Then on the CSC 517, Fall 2017 Row, click on "Add Participants" in the actions column. This is also indicated by a blue shirted person.


Once in there, copy on of the student names (such as student7487) and then go to Manage... -> Impersonate User and enter their name (student7487) into the text field.


After impersonating them, you should be able to see their assignments. Click on any of their assignments such as "Program 1", then click on "Alternate View" to the right of "Your Scores".


Modified Files

reputation_web_service_controller.rb

New Files

reputation_web_service_controller_spec.rb

Team Information

Project Mentor:
Saurabh Shingte

Project Members:
Abhishek Gupta
Skieler Capezza
Ummu Kolusum Yasmin Ahamed Adam

References

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2020_-_E2064._Refactor_reputation_web_service_controller.rb&oldid=135731"