CSC/ECE 517 Fall 2017/E1756 TLD Refactor response.rb: Difference between revisions
No edit summary |
No edit summary |
||
| Line 123: | Line 123: | ||
The method display_as_html is complex. Many functions are embedded in to a single method. | The method display_as_html is complex. Many functions are embedded in to a single method. | ||
[[File: | |||
[[File:E1756_1.png]] | |||
Solution: | Solution: | ||
| Line 129: | Line 130: | ||
The method has been broken in to smaller and simpler parts and have been given the reasonable names that is | The method has been broken in to smaller and simpler parts and have been given the reasonable names that is | ||
[[File: | [[File:E1756_2.png]] | ||
[[File: | [[File:E1756_3.png]] | ||
For more details, please go to the link | For more details, please go to the link | ||
| Line 139: | Line 140: | ||
The method has repeated lines of code. | The method has repeated lines of code. | ||
[[File: | [[File:E1756_4.png]] | ||
Solution: | Solution: | ||
| Line 145: | Line 146: | ||
The loop has been included in the method instead of writing the same lines of code. | The loop has been included in the method instead of writing the same lines of code. | ||
[[File: | [[File:E1756_5.png]] | ||
For more details, please go to the link | For more details, please go to the link | ||
| Line 153: | Line 154: | ||
Changing the method names | Changing the method names | ||
[[File: | [[File:E1756_6.png]] | ||
| Line 170: | Line 171: | ||
spec/models/response_spec.rb | spec/models/response_spec.rb | ||
[[File: | [[File:E1756_7.png]] | ||
For more details, please go to the link | For more details, please go to the link | ||
| Line 178: | Line 179: | ||
Use sort_by(&:seq) instead of sort { |a, b| a.seq <=> b.seq } | Use sort_by(&:seq) instead of sort { |a, b| a.seq <=> b.seq } | ||
[[File: | [[File:E1756_8.png]] | ||
Solution: | Solution: | ||
[[File: | [[File:E1756_9.png]] | ||
For more details, please go to the link | For more details, please go to the link | ||
| Line 190: | Line 191: | ||
Use find_by instead of where.first | Use find_by instead of where.first | ||
[[File: | [[File:E1756_10.png]] | ||
Solution: | Solution: | ||
[[File: | [[File:E1756_11.png]] | ||
For more details, please go to the link | For more details, please go to the link | ||
Revision as of 01:39, 2 November 2017
About Expertiza
Expertiza is a Ruby on Rails based Open Source project. It is a collaboration tool which lets users with different roles (student, instructor, teaching assistant) to collaborate on a course in an institution. A collaboration could be for an assignment where students teams up for an assignment and instructors grades them on the basis of their submission. Students could review other's works and give feedbacks as well.
Github link
https://github.com/expertiza/expertiza
Wiki link
http://wiki.expertiza.ncsu.edu/index.php/Expertiza_documentation
Problem Statement
Background
response.rb is the default class to interact with the responses table, which stores all answers of each questionnaire. It is basically the alternate view. The contents get changed based on the user interaction with the application. Basically there are two important views, one for students and the other is for instructors. Based on the type of the user, the contents of the page get changed.
What’s wrong with it?
response.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. The previous project did not contain any test cases for the response.rb model. The response_spec.rb file did not contain any test cases except the mocks.
Problems
Problem 1
Refactor display_as_html method
Write failing tests first
Split into several simpler methods and assign reasonable names
Extract duplicated code into separate methods
Problem 2
Refactor self.get_volume_of_review_comments method
Write failing tests first
Convert duplicated code into loop
Problem 3
Rename method and change all other places it is used
Write failing tests first
get_total_score → total_score
get_average_score → average_score
get_maximum_score → maximum_score
Problem 4
Use sort_by(&:seq) instead of sort { |a, b| a.seq <=> b.seq }
Write failing tests first
Problem 5
Use find_by instead of where.first
Write failing tests first
Problem 6
Complete the pending tests in grades_controller_spec.rb, and write integration tests for newly-created methods. Refactor corresponding methods, and only then finish pending tests.
Refactoring Decision
Decision 1
The display_as_html method has been modified. The logic which was there in the display_as_html method has been put in to two different methods called construct_instructor_html and construct_student_html files. One of these methods are called based on the value of the id. The logic of adding additional comment which was in the display_as_html method has been included in another method called construct_review_response and it is called from the display_as_html method.
Decision 2
The get_volume_of_review_comments method has been modified by putting the repeated statements in to a loop which runs on variable i. The variable comments_in_round_i changes based on the value of the variable i.
Decision 3
The methods get_total_score, get_average_score and get_maximum_score have been changed to total_score,average_score and maximum_score. The changes have been made in all the places wherever these methods are called.
Decision 4
The method 'where' in significant_difference? method has been changed to find_by.
Decision 5
The sort method which was in display_as_html method has been changed to sort_by. The sort_by method is present in construct_review_response refactored method.
Files Modified
Problem 1
app/models/response.rb
Problem 2
app/models/response.rb
Problem 3
app/controllers/assessment360_controller.rb
app/controllers/popup_controller.rb
app/helpers/review_mapping_helper.rb
app/models/collusion_cycle.rb
spec/models/response_spec.rb
Problem 4
app/models/response.rb
Problem 5
app/models/response.rb
Issue and Solution
Problem 1:
The method display_as_html is complex. Many functions are embedded in to a single method.
Solution:
The method has been broken in to smaller and simpler parts and have been given the reasonable names that is
For more details, please go to the link
Problem 2:
The method has repeated lines of code.
Solution:
The loop has been included in the method instead of writing the same lines of code.
For more details, please go to the link
Problem 3:
Changing the method names
Solution:
The files that are modified are:
app/controllers/assessment360_controller.rb
app/controllers/popup_controller.rb
app/helpers/review_mapping_helper.rb
app/models/collusion_cycle.rb
spec/models/response_spec.rb
For more details, please go to the link
Problem 4:
Use sort_by(&:seq) instead of sort { |a, b| a.seq <=> b.seq }
Solution:
For more details, please go to the link
Problem 5:
Use find_by instead of where.first
Solution:
For more details, please go to the link
Test Plan
The pre conditions which are required to test the methods are not applicable because the project includes the process of refactoring and testing the refactored methods.
The response_spec.rb file does not include any edge cases. The edge cases are out of scope because the project includes refactoring and writing test cases.
| Methods | Contexts |
|---|---|
| response_id | tests if the response id is returned |
| display_as_html | tests if corresponding html page is rendered based on the id passed |
| construct_instructor_html | not tested |
| construct_student_html | not tested |
| construct_review_response | not tested |
| add_table_rows | |
| additional_comment | |
| total_score | tests if the method computes the total score of the review |
| delete | |
| average_score | tests if the method computes the average score when the other function computes the maximum score |
| maximum_score | tests if the method returns the maximum score of the current review |
| tests if there is a call to email method in corresponding response maps | |
| questionnaire_by_answer | tests if method returns the questionnaire of the question of current answer or review questionnaire of current assignment based on the value of the answer |
| concatenate_all_review_comments | tests if the method returns concatenated review comments |
| get_volume_of_review_comments | tests if the method returns volumes of review comments in each round |
| significant_difference? | tests if the method returns true when the difference between average score on same artifact from others and current score is bigger thatn allowed percentage otherwise false |
| avg_scores_and_count_for_prev_reviews | tests if the method returns the average score and count of previous reviews when current response is not in current response array |
| notify_instructor_on_difference |