CSC/ECE 517 Fall 2017/E1772 Refactor reputation web service controller.rb: Difference between revisions
No edit summary |
|||
(31 intermediate revisions by 2 users not shown) | |||
Line 1: | Line 1: | ||
==Introduction== | =='''Introduction'''== | ||
==='''Introduction to Expertiza'''=== | ==='''Introduction to Expertiza'''=== | ||
[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. | ||
Line 28: | Line 28: | ||
---- | |||
=='''NewFiles'''== | |||
reputation_web_service_controller_spec.rb | |||
---- | |||
=='''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> | |||
[https://github.com/expertiza/expertiza/pull/1027 Github Pull Request] | |||
---- | ---- | ||
=='''Approach taken to resolve the issues'''== | =='''Approach taken to resolve the issues'''== | ||
==='''Replace class variable with a class instance var, and change all other places using these variables.'''=== | ==='''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 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. | In the project, we are asked to replace the class variable with a class instance variables. | ||
Line 54: | Line 69: | ||
---- | ---- | ||
==='''Refactor method db_query'''=== | ==='''Refactor method db_query'''=== | ||
Line 64: | Line 78: | ||
end | end | ||
</pre> | </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. | 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:Reputation8.png]] | ||
And the same problems occurs when we assigns value to variable ''@results''. We also need to change it. | And the same problems occurs when we assigns value to variable '''''@results'''''. We also need to change it. | ||
[[File:Reputation10.png]] | [[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: | 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: | ||
[[File:Reputation9.png]] | [[File:Reputation9.png]] | ||
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]] | |||
==='''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: | |||
[[File:Reputation12.png]] | |||
[[File:Reputation13.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.'''=== | |||
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:refactor21.png]] | |||
[[File:refactor19.png]] | |||
==='''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 | |||
[[File:refactor22.png]] | |||
[[File:refactor23.png]] | |||
==='''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 | |||
<pre> | |||
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 | |||
</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> | |||
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 | |||
</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: | |||
@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. |
Latest revision as of 00:47, 6 November 2017
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.
- 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
Modified Files
reputation_web_service_controller.rb
NewFiles
reputation_web_service_controller_spec.rb
Important links
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.