CSC/ECE 517 Fall 2017/E1756 TLD Refactor response.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
No edit summary
 
(31 intermediate revisions by the same user not shown)
Line 3: Line 3:
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.
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'''
===Github link===


     https://github.com/expertiza/expertiza  
     https://github.com/expertiza/expertiza  


'''Wiki link'''
===Wiki link===


     http://wiki.expertiza.ncsu.edu/index.php/Expertiza_documentation
     http://wiki.expertiza.ncsu.edu/index.php/Expertiza_documentation
== Team Members ==
Bhavik Patel
Arpita Shekhar
Amulya Deepak Varote


== Problem Statement ==
== Problem Statement ==


'''Background'''
===Background===


response.rb is the default class to interact with the responses table, which stores all answers of each questionnaire.
response.rb is the default model to interact with the responses table, which stores the answers to every questionnaire. It is basically the alternate view. The content is displayed based on the user role. There are two important views, one for students and the other for instructors.


'''What’s wrong with it?'''
'''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.
response.rb is a fairly complex file. It contains lots 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 response_spec.rb did not contain any test cases for the response.rb model except the mocks.
 
'''Problems'''


===Problems===


''Problem 1:Refactor display_as_html method''
====Problem 1====
''Refactor display_as_html method''


  Write failing tests first
  Write failing tests first
Line 32: Line 40:
  Extract duplicated code into separate methods
  Extract duplicated code into separate methods


''Problem 2:Refactor self.get_volume_of_review_comments method''
====Problem 2====
''Refactor self.get_volume_of_review_comments method''


  Write failing tests first
  Write failing tests first
Line 38: Line 47:
  Convert duplicated code into loop
  Convert duplicated code into loop


''Problem 3:Rename method and change all other places it is used''
====Problem 3====
''Rename method and change all other places it is used''


  Write failing tests first
  Write failing tests first
Line 48: Line 58:
  get_maximum_score → maximum_score
  get_maximum_score → maximum_score


''Problem 4:Use sort_by(&:seq) instead of sort { |a, b| a.seq <=> b.seq }''
====Problem 4====
''Use sort_by(&:seq) instead of sort { |a, b| a.seq <=> b.seq }''


  Write failing tests first
  Write failing tests first


''Problem 5:Use find_by instead of where.first''
====Problem 5====
''Use find_by instead of where.first''


Write failing tests 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.''






'''Issue links (external)'''
==Refactoring Decision==
 
===Decision 1===
 
The display_as_html method has been modified. The existing logic for display_as_html method has been refactored into two different methods called construct_instructor_html and construct_student_html. Both these methods are generating html content to be displayed based on the identifier parameter passed to the function. The logic of displaying review, which was earlier in the display_as_html method has been included in another method called construct_review_response.
 
===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 names are now created based on the number of reviews.
 
===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.


    https://github.com/expertiza/expertiza/issues/720
===Decision 4===
    https://github.com/expertiza/expertiza/issues/732


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 ==
== Files Modified ==


For Problem 1:
===Problem 1===


''app/models/response.rb''
''app/models/response.rb''


For Problem 2:
===Problem 2===


''app/models/response.rb''
''app/models/response.rb''


For Problem 3:
===Problem 3===


''app/controllers/assessment360_controller.rb''
''app/controllers/assessment360_controller.rb''
Line 89: Line 114:
''spec/models/response_spec.rb''
''spec/models/response_spec.rb''


For Problem 4:
===Problem 4===


''app/models/response.rb''
''app/models/response.rb''


For Problem 5:
===Problem 5===


''app/models/response.rb''
''app/models/response.rb''
Line 103: Line 128:
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:E1756pic1.png]]
----
 
[[File:E1756_1.png]]
 
----


Solution:
Solution:


The method has been broken in to smaller and simpler parts and have been given the reasonable names  
The method has been broken in to smaller and simpler parts and have been given the reasonable names that is
 
----
 
[[File:E1756_2.png]]


[[File:E1756pic2.png|200px|thumb|left|alt text]]
----


[[File:E1756_3.png]]


[[File:E1756pic3.png|200px|thumb|left|alt text]]
----
 
For more details, please go to the link
 
https://github.com/arpitashekhar/expertiza/commit/2e022e66b8d46dc8e7a7e9aae78420837b89f045


Problem 2:
Problem 2:
Line 118: Line 156:
The method has repeated lines of code.
The method has repeated lines of code.


[[File:E1756pic4.png]]
----
 
[[File:E1756_4.png]]
 
----


Solution:
Solution:
Line 124: Line 166:
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:E1756pic5.png]]
----
 
[[File:E1756_5.png]]
 
----
 
For more details, please go to the link
 
https://github.com/arpitashekhar/expertiza/commit/cce1c6bfdf175422a7bd075b21152f7b1839cf7b


Problem 3:
Problem 3:
Line 130: Line 180:
Changing the method names
Changing the method names


[[File:E1756pic6.png]]
----
 
[[File:E1756_6.png]]
 
----
 
Solution:
Solution:
[[File:E1756pic7.png]]
 
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
 
----
 
[[File:E1756_7.png]]
 
----
 
For more details, please go to the link
 
https://github.com/arpitashekhar/expertiza/commit/07db41df233ea9bda4f8515ac62bff012055bf2d


Problem 4:
Problem 4:


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:E1756pic8.png]]
 
----
 
[[File:E1756_8.png]]
 
----
 
Solution:
Solution:


[[File:E1756pic9.png]]
----
 
[[File:E1756_9.png]]
 
----
 
For more details, please go to the link
 
https://github.com/arpitashekhar/expertiza/commit/35d34e7b8fca2f1a24fdfecdd601ab4390eada20


Problem 5:
Problem 5:


find_by instead of where.first
Use find_by instead of where.first


[[File:E1756pic10.png]]
----
 
[[File:E1756_10.png]]
 
----


Solution:
Solution:
[[File:E1756pic11.png]]
 
----
 
[[File:E1756_11.png]]
 
----
 
For more details, please go to the link
 
https://github.com/arpitashekhar/expertiza/commit/c3d5b7913e488240276d6ee023c33fcaf3deb634
 
 
 
==Testing==
 
The test cases of response.rb file have been implemented. All the contexts of response_spec.rb pass with the overall coverage of 27.16%.
 
----
 
[[File:test_result.png]]
 
----
 
For the test cases, please go the link
 
https://github.com/arpitashekhar/expertiza/blob/E1756/spec/models/response_spec.rb
 
==Test Plan==
The pre conditions and edge cases for test scenarios are not applicable because the project includes refactoring and testing the refactored methods.
 
{| class="wikitable"
|-
! 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
|-
| total_score
| tests if the method computes the total score of the review
|-
| average_score
| tests if the method computes the average score based on the total and maximum scores of the review
|-
| maximum_score
| tests if the method returns the maximum score of the current review
|-
| email
| tests if there is a call to email method in corresponding response map model
|-
| questionnaire_by_answer
| tests if method returns the questionnaire 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 scores of current and previous round of review is significant and returns false otherwise
|-
| avg_scores_and_count_for_prev_reviews
| tests if the method returns the average score and count of previous reviews
|-
|}
 
==Manual testing steps==
 
1. Login as an instructor. Create a new course and create a new team/individual assignment for that course.
 
2. Add participants to the assignment created.
 
3. Set the submission and review deadlines, number of submissions, rubrics for reviews, number of reviews and all the other fields for the assignment.
 
4. Login as a student_1, who has been added as a participant in the created assignment.
 
5. Now the student_1 will be able to see the assignment, form the team and submit the work.
 
6. After other students also submit their work, student_1 will be able to review others work and see his reviews given by others.
 
7. The page which displays the reviews is linked to the model which we have refactored(response.rb).
 
In order to verify the refactoring, please perform the above steps before and after executing our code.
 
==Video which shows refactoring and implemented test cases==
   
        https://drive.google.com/file/d/0Bw76yptzkQDJNDZyb2kwazlwSGM/view?usp=sharing
 
==Other links==
 
===Pull request link===
 
        https://github.com/expertiza/expertiza/pull/1047

Latest revision as of 04:00, 3 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

Team Members

Bhavik Patel

Arpita Shekhar

Amulya Deepak Varote

Problem Statement

Background

response.rb is the default model to interact with the responses table, which stores the answers to every questionnaire. It is basically the alternate view. The content is displayed based on the user role. There are two important views, one for students and the other for instructors.

What’s wrong with it?

response.rb is a fairly complex file. It contains lots 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 response_spec.rb did not contain any test cases for the response.rb model 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


Refactoring Decision

Decision 1

The display_as_html method has been modified. The existing logic for display_as_html method has been refactored into two different methods called construct_instructor_html and construct_student_html. Both these methods are generating html content to be displayed based on the identifier parameter passed to the function. The logic of displaying review, which was earlier in the display_as_html method has been included in another method called construct_review_response.

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 names are now created based on the number of reviews.

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

https://github.com/arpitashekhar/expertiza/commit/2e022e66b8d46dc8e7a7e9aae78420837b89f045

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

https://github.com/arpitashekhar/expertiza/commit/cce1c6bfdf175422a7bd075b21152f7b1839cf7b

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

https://github.com/arpitashekhar/expertiza/commit/07db41df233ea9bda4f8515ac62bff012055bf2d

Problem 4:

Use sort_by(&:seq) instead of sort { |a, b| a.seq <=> b.seq }

Solution:

For more details, please go to the link

https://github.com/arpitashekhar/expertiza/commit/35d34e7b8fca2f1a24fdfecdd601ab4390eada20

Problem 5:

Use find_by instead of where.first

Solution:

For more details, please go to the link

https://github.com/arpitashekhar/expertiza/commit/c3d5b7913e488240276d6ee023c33fcaf3deb634


Testing

The test cases of response.rb file have been implemented. All the contexts of response_spec.rb pass with the overall coverage of 27.16%.

For the test cases, please go the link

https://github.com/arpitashekhar/expertiza/blob/E1756/spec/models/response_spec.rb

Test Plan

The pre conditions and edge cases for test scenarios are not applicable because the project includes refactoring and testing the refactored methods.

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
total_score tests if the method computes the total score of the review
average_score tests if the method computes the average score based on the total and maximum scores of the review
maximum_score tests if the method returns the maximum score of the current review
email tests if there is a call to email method in corresponding response map model
questionnaire_by_answer tests if method returns the questionnaire 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 scores of current and previous round of review is significant and returns false otherwise
avg_scores_and_count_for_prev_reviews tests if the method returns the average score and count of previous reviews

Manual testing steps

1. Login as an instructor. Create a new course and create a new team/individual assignment for that course.

2. Add participants to the assignment created.

3. Set the submission and review deadlines, number of submissions, rubrics for reviews, number of reviews and all the other fields for the assignment.

4. Login as a student_1, who has been added as a participant in the created assignment.

5. Now the student_1 will be able to see the assignment, form the team and submit the work.

6. After other students also submit their work, student_1 will be able to review others work and see his reviews given by others.

7. The page which displays the reviews is linked to the model which we have refactored(response.rb).

In order to verify the refactoring, please perform the above steps before and after executing our code.

Video which shows refactoring and implemented test cases

        https://drive.google.com/file/d/0Bw76yptzkQDJNDZyb2kwazlwSGM/view?usp=sharing

Other links

Pull request link

        https://github.com/expertiza/expertiza/pull/1047
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1756_TLD_Refactor_response.rb&oldid=111690"