Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-12-09T19:06:58Z

Hrmauny: /* Work Plan */

==Problem Statement==
Our project concentrates on making things readable and understandable in <code>app/helpers/review_mapping_helper.rb</code> file. We have have refactored some methods mentioned below for better understanding while maintaining the functionality. The aim is to also make it better understandable for other developers by adding comments wherever necessary.

==Team==
===Mentor===
Devashish Vachhani (dvachha@ncsu.edu)

===Team Members===
Harsh Mauny (hrmauny@ncsu.edu) 
Connor Davidson (cdavids@ncsu.edu) 
Aditya Joshi (ajoshi25@ncsu.edu) 

==Issues to be Addressed==

* '''Issue #1:'''The next several methods generate charts. They are cohesive enough that they should be in their own separate file, either another helper or a mixin.

* '''Issue #2:'''Various method names begin with get. This is not Ruby-like. Change the names to something more appropriate.

* '''Issue #3:'''There are several methods for feedback_response_maps. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.

* '''Issue #4:'''The method sort_reviewer_by_review_volume_desc should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #6:'''The file ends with three small classes being defined. There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. Refactor this method to make it more readable.

==Plan of work==
We started our work by analysing the <code>review_mapping_helper.rb</code> how it links to rest of the code and discussing the flow of the project in detail with our mentor. Based on our understanding of the code we divided our project requirements in terms of difficulty levels as shown in the [https://docs.google.com/document/d/1AmXVfhH3icn8Pc9oJ3U7okER2NhBDIFFtFxTofmrKcE/edit?usp=sharing document]. then we distributed the tasks among us and started refactoring various methods and creating new files as and when necessary. We also ensured the test cases passing earlier were intact and added new ones if necessary.

=='''Project 4 - DESIGN DOC'''==
Following are the changes we have been assigned to do after completion of project 3

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow
Apart from these issues we have added some issues of our own based on the feedback of project 3 review which are as follows

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. Refactor this method to make it more readable.

* A lot of variables are difficult to understand and hence we plan to refactor the variable for better readability and understanding
* We were not able to increase the code coverage after our changes during project 3. Here in project 4 we want to focus on writing more tests and using the test skeleton.

Below is an UML diagram already uploaded in Expertiza database documentation for reader reference 
[[File:Review mappings imported.png|700px]] 

====Design Patterns====
The Code present in review_mapping_helper can be improved by using design patterns to improve maintainability, readability, and flexibility.
Following are some design patterns that are suggested

* Methods can be extracted into smaller, well maintained with just a single responsibility. This will promote code readability and promote code reuse.

*The methods review_report_data, team_color, obtain_team_color, and others follow a common sequence of steps but allow for variation in some steps. Applying the Template Method Pattern can help define a common structure in a base method while allowing specific steps to be implemented in subclasses or overridden in derived methods.

*Descriptive and short naming conventions are required so the methods and variables are self explanatory.

* Separation of Concerns should be followed for following code.

====Work Plan====

The plan of work on the above issues is as follows 

*'''Issue #5:''' There is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
'''Methods involved:''' <code> list_review_submissions </code> 
'''Problem:''' following are the problems here as stated by the task 
* Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
[[File:E2353-issue-5-orignal-issue.png|700px]] 
'''Solution:''' Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why previous students implemented this method
* Reading the code and the usages of this method to understand how it's being used
* better variable and method names if needed.
 

*'''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow 
'''Methods involved:''' <code> get_data_for_review_report </code> 
'''Problem:''' following are the problems here as stated by the task 
* There are several methods for feedback_response_maps.
* It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing. 
[[File:E2353-issue-7-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why this method was originally implemented
* Reading the code and the usages of this method to understand how it's being used
* refactor to make better variable and method names if necessary.
 

*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American word color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green. 
[[File:E2353-issue-8-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The colour word has to be replaced by color for consistency
* In order to improve the readability we can reduce the cognitive complexity and branching by using less if-else block and further dividing the method into multiple block.
* More comments can be added to make sure the reader understands what each line does in these methods.
* make sure proper color get assigned based on the review status by making sure all tests pass after refactoring and maybe new test cases can be added
* there are some unwanted array data structure used in order to pass the color code which should be removed to reduce complexity.
* better variable and method names if needed.
 

* '''Issue #9:''' get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. Refactor this method to make it more readable.
'''Methods involved:''' <code>get_awarded_review_score</code> 
'''Problem:''' following are the problems here as stated by the task 
* The method computes an overall score based upon scores awarded in individual rounds, but the score calculation code is not readable. 
[[File:E2353-issue-9-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The score calculation code needs to be understood thoroughly and as stated, has to be refactored in the <code>get_awarded_review_score</code> method
* We will add comments as necessary once in the review mapping helper so that readers can understand what the code fragments are doing
* The current variable names will be improved
 

==Files Modified==

* app/helpers/charts_helper.rb
* app/helpers/review_mapping_helper.rb
* app/helpers/review_mapping_helper.rb
* app/views/reports/_calibration_report.html.erb
* app/views/reports/_feedback_report.html.erb
* app/views/reports/_review_report.html.erb
* app/views/reports/_team_score.html.erb
* app/views/reports/_team_score_score_awarded.html.erb
* spec/features/review_mapping_helper_spec.rb
* spec/helpers/review_mapping_helper_spec.rb

==Project tracking==
please use this template
*'''Issue #[number]:''' text 
'''Methods involved:''' <code>text</code> 
'''Problem:''' text 
'''Solution:'''text, code, images if any 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Name
 

'''This section contains combined changes of project 3 and 4'''
----

* '''Issue #1:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
'''Methods involved:''' <code>initialize_chart_elements()</code>, <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code>, <code>calculate_key_chart_information()</code> 
'''Previous Code:''' All these methods were previously a part of the <code>review_mapping_helper.rb</code> when it should have its separate helper file 
'''Problem:''' These methods perform similar tasks of generating graphs and can have there separate module or a mixin and also each of the methods more than 30 lines inside them making them too large 
'''Solution:''' distributed this code among 2 newly created files named <code>review_mapping_charts_helper.rb</code> and <code>data_mapping_helper.rb</code> within helper folder. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. <code>review_mapping_charts_helper.rb</code> contains the original methods and <code>data_mapping_helper.rb</code> manages the data need by these chart methods 

'''review_mapping_charts_helper.rb'''
<pre>
module ReviewMappingChartsHelper
...
def initialize_chart_elements()
def display_volume_metric_chart()
def display_tagging_interval_chart()
def calculate_key_chart_information()
</pre>
 
'''all link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #2:''' change methods names starting with "get" to something appropriate 
'''Methods involved:''' <code> get_data_for_review_report()</code>, <code> get_team_color()</code>, <code> get_link_updated_at()</code>, <code> get_team_reviewed_link_name()</code>, <code> get_awarded_review_score()</code>, <code> get_each_review_and_feedback_response_map()</code>, <code> get_certain_review_and_feedback_response_map()</code>, <code> get_css_style_for_calibration_report()</code> 
'''Previous Code:''' previous code used "get" in method names 
'''Problem:''' All these methods used "get_" which is not ruby-like. 
'''Solution:''' change method names to a more ruby-like name that was appropriate for the given context 

'''Contributor:''' Connor Davidson
 
----
*'''Issue #3:''' Several methods for feedback_response_maps, check docs to manage this confusion 
'''Methods involved:''' <code> feedback_response_for_author()</code> 
'''Previous Code:''' previous code had an ambiguous variable name 
'''Problem:''' It wasn't clear what the variable was used for. 
'''Solution:''' refactor the variable name to be more easily understood and add comments to give a clear explanation for why it exists 

'''Contributor:''' Connor Davidson
 
----

*'''Issue #4:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
'''Problem:''' Currently the method sort_reviewer_by_review_volume_desc sorts the reviewers in descending order but only considers one metric which is review volume which makes it very specific and it cannot be used if we want to sort by other metrics 
'''Solution:'''The solution here is make this a generalized method by renaming <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and accepting a metric as argument on which we can sort the the reviewers as shown below. Since it was used in <code>review_report</code> partial the implementation was changed there as well 
'''review_mapping_helper.rb''' 
[[File: E2353-Issue-4-1.png|900px]] 
'''app/views/reports/_review_report.html.erb''' 
[[File:E2353issue-4-2.png|900px]] 
'''review_mapping_helper_spec.rb''' 
[[File:E2353-issue4-3.png|900px]] 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-1f05f505eaa5edaf0790cdeb6e774cc499685cce4edf12b253ecd358ce710582 review_mapping_helper_spec.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #5:''' The method <code>list_review_submissions</code> is not clear in terms of its utility
'''Methods involved:''' <code>list_review_submissions</code> 
'''Problem:''' It is not clear what the method does, where its used and if it can be replaced. 
'''Solution:''' Comments (shown below) have been added to explain the usage of the method both at the method call in the partial <code>_review_report.html.erb</code> and the method definition in <code>review_mapping_helper.rb</code>

<code>review_mapping_helper</code> code snippet
<pre>
# Extracts the files submitted for a particular pair of participant and reviewee
def list_review_submissions(participant_id, reviewee_team_id, response_map_id)
participant = Participant.find(participant_id)
team = AssignmentTeam.find(reviewee_team_id)
html = ''
unless team.nil? || participant.nil?
# Build a path to the review submissions using the team's path and the response map ID
review_submissions_path = team.path + '_review' + '/' + response_map_id.to_s
files = team.submitted_files(review_submissions_path)
html += display_review_files_directory_tree(participant, files) if files.present?
</pre>

<code>_review_report</code> code snippet
<pre>
# Following Table entry displays review submissions for specific reviewer and reviewee
<%= list_review_submissions(reviewer.id, reviewer_map.reviewee_id, reviewer_map.id) %>


<%= list_hyperlink_submission(reviewer_map.id, 5386) if Assignment.find_by(id: @id.to_i).try(:course).try(:instructor).try(:name) == 'Jkidd'%>
</div>
</pre>

'''Justification for keeping the method:''' 
-> The method has been added to the helper to reduce the complexity of logic in the views which is inherently good programming practice 
-> It builds the parameters required to feed into <code>display_review_files_directory_tree</code> method - a piece of logic that otherwise would have reduced the readability of the code in the partial 
-> Although the helper is just used at one place, its necessary to generate review report tables and hence seems to be the appropriate solution in terms of both functionality and code readability 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
'''Contributor:''' Aditya Joshi
 
----

*'''Issue #6:''' The Last 3 Classes in review_helper need to be reviewed 
'''Methods involved:''' <code>initialize</code>, <code>reviews_needed</code>, <code>reviews_per_team</code>, <code>reviews_per_student</code> (separate methods for both the sub classes) 
'''Problem:''' The file ends with three small classes being defined. There are no comments at all to explain what is being done 
'''Solution:'''The usage of the three classes was traced back to assignment views and ReviewMappingController. It is evident that these classes have been used to encapsulate the logic for different review strategies in the system. Some comments have been added to explain the functionality of these classes. 
'''review_strategy classes''' 
[[File:E2353-Issue-6.png|900px]] 
'''Further explanation of the methods:''' 
<code>reviews_needed</code> : Calculates the total number of reviews needed for all teams/participants 
<code>reviews_per_team</code> : Calculates the number of reviews each team should receive 
<code>reviews_per_student</code> (for StudentReviewStrategy) : Specifies the number of reviews each student should perform 
<code>reviews_per_student</code> (for TeamReviewStrategy) : Specifies the number of reviews each student should perform based on team assignments 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Aditya Joshi
 
----

'''Issue #7''': method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed.
'''Methods involved:''' <code> get_data_for_review_report </code>
'''Problem:''' The method is returning two values and the purpose for the method is ambiguous 
'''Solution:''' Break this method into two separate methods that return each of the component parts of the ambiguous return value 
'''Further explanation of the methods:''' 
<code> get_data_for_review_report </code> : Returned an array of two distinct values. This method was refactored into two separate methods that each return one of the values from the initial state. 

'''All links related to these changes:''' 
[https://github.com/adityajoshi1114/expertiza/pull/16] 
'''Contributor:''' Connor Davidson
 

----
*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green.
Below is the original code
<pre>
def team_color(response_map)
# Storing redundantly computed value in a variable
assignment_created = @assignment.created_at
# Storing redundantly computed value in a variable
assignment_due_dates = DueDate.where(parent_id: response_map.reviewed_object_id)
# Returning color based on conditions
if Response.exists?(map_id: response_map.id)
if !response_map.try(:reviewer).try(:review_grade).nil?
'brown'
elsif response_for_each_round?(response_map)
'blue'
else
obtain_team_color(response_map, assignment_created, assignment_due_dates)
end
else
'red'
end
end

# loops through the number of assignment review rounds and obtains the team color
def obtain_team_color(response_map, assignment_created, assignment_due_dates)
color = []
(1..@assignment.num_review_rounds).each do |round|
check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
end
color[-1]
end

# checks the submission state within each round and assigns team color
def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
if submitted_within_round?(round, response_map, assignment_created, assignment_due_dates)
color.push 'purple'
else
link = submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates)
if link.nil? || (link !~ %r{https*:\/\/wiki(.*)}) # can be extended for github links in future
color.push 'green'
else
link_updated_at = get_link_updated_at(link)
color.push link_updated_since_last?(round, assignment_due_dates, link_updated_at) ? 'purple' : 'green'
end
end
end
</pre>
 

'''Solution:''' First we understood the color coding scheme based on the references provided to us and based on which the code was refactored to address the problem. Below are the changes that were made
* The colour word was replaced by color for consistency
* In order to improve the readability the cognitive complexity and branching was reduced by using less if-else block and further dividing the method into multiple block.
* comments were added providing an explanation of what the code does
* making sure proper color get assigned based on the review status
* better variable and method names wherever needed. 
[[File:E2353-issue-8-updated-team-color.png|700px]] 
'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny

----

*'''Issue #9:''' Method <code>get_awarded_review_score</code> needs to be refactored
'''Methods involved:''' <code>get_awarded_review_score</code> 
'''Solution:''' Post tackling of Issue 2, the name of the method was changed to <code>compute_awarded_review_score</code> to make it more ruby like. Further on, the code which involved redundant setting of instance variables was placed inside the loop to make it more concise. The explicit check for nil or -1.0 was removed and added to the <code>instance_variable_set</code> line.

Before
<pre>
def compute_awarded_review_score(reviewer_id, team_id)
# Storing redundantly computed value in num_rounds variable
num_rounds = @assignment.num_review_rounds
# Setting values of instance variables
(1..num_rounds).each { |round| instance_variable_set('@score_awarded_round_' + round.to_s, '-----') }
# Iterating through list
(1..num_rounds).each do |round|
# Changing values of instance variable based on below condition
if @review_scores[reviewer_id] && @review_scores[reviewer_id][round] && @review_scores[reviewer_id][round][team_id] && @review_scores[reviewer_id][round][team_id] != -1.0
instance_variable_set('@score_awarded_round_' + round.to_s, @review_scores[reviewer_id][round][team_id].to_s + '%')
end
end
end
</pre>

After
<pre>
def compute_awarded_review_score(reviewer_id, team_id)
num_rounds = @assignment.num_review_rounds

(1..num_rounds).each do |round|
score = @review_scores && @review_scores[reviewer_id] && @review_scores[reviewer_id][round] && @review_scores[reviewer_id][round][team_id]
instance_variable_set("@score_awarded_round_#{round}", "#{score}%") unless score.nil? || score == -1.0
end
end
</pre>

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/commits/7549f7eb407f01fa7afb84ac6a2581bf36b13396 review_mapping_helper.rb] 
'''Contributor:''' Aditya Joshi
----

==Test Plan==
This is a refactoring project, so it is expected that the original functionality should be retained. Some of the task involved adding comments to previous code, changing variable name etc. which no change was required in test cases. Some methods where method names were changed or they were shifted to other files, for these methods necessary changes were made in respective Rspec files. All the test cases passed which implies that the refactored code did not break the existing functionality.
 

After Project 3 we have changed some method and variable names to improve readability of <code>review_mapping_helper.rb</code> and relevant file and in turn made appropriate changes in respective test files to make sure all test cases are passing. Some new test cases have been added as suggested by test skeleton files in <code>review_mapping_helper_spec.rb</code> example of some of the test cases are below 
<pre>
describe 'test calculate_key_chart_information' do
it 'should return new Hash if intervals are not empty' do
intervals = [1.00, 2.00, 3.00, 4.00, 5.00, 6.00]
result = helper.calculate_key_chart_information(intervals)
expect(result).to be_a_kind_of(Hash)
expect(result[:mean]).to eq(3.50)
expect(result[:min]).to eq(1.00)
expect(result[:max]).to eq(6.00)
expect(result[:variance]).to eq(2.92)
expect(result[:stand_dev]).to eq(1.71)
end
it 'returns the mean, min, max, variance, and standard deviation of the intervals' do
expect(calculate_key_chart_information([10, 15, 20])).to eq(mean: 15.0, min: 10, max: 20, variance: 16.67, stand_dev: 4.08)
expect(calculate_key_chart_information([5, 8, 12, 15, 20])).to eq(mean: 12.0, min: 5, max: 20, variance: 27.6, stand_dev: 5.25)
end
context 'when intervals are empty' do
it 'returns an empty hash' do
# Test case 4
expect(calculate_key_chart_information([])).to eq({})
end
end
context 'when intervals contain values greater than the threshold' do
it 'ignores those intervals and returns an empty hash' do
expect(calculate_key_chart_information([60, 45, 35])).to eq({})
end
end
end
</pre>

<pre>
describe 'test calibration_report_css_class' do
context 'when the difference is 0' do
it 'returns c5 as the CSS class' do
css0 = helper.calibration_report_css_class(0)
expect(css0). to eq('c5')
end
end
context 'when the difference is 1' do
it 'returns c4 as the CSS class' do
css1 = helper.calibration_report_css_class(-1)
expect(css1). to eq('c4')
end
end
context 'when the difference is 2' do
it 'returns c3 as the CSS class' do
css2 = helper.calibration_report_css_class(-2)
expect(css2). to eq('c3')
end
end
context 'when the difference is 3' do
it 'returns c2 as the CSS class' do
css3 = helper.calibration_report_css_class(-3)
expect(css3). to eq('c2')
end
end
context 'when the difference is greater than 3' do
it 'returns c1 as the CSS class' do
css4 = helper.calibration_report_css_class(6)
expect(css4). to eq('c1')
end
end
end
</pre>
 The image below shows the test cases are passing as well as most of the code climate issues were resolved. 
[[File:E2353-Test-Report.png|250px]]

==Github Links==
===Link to Expertiza Repository===
https://github.com/expertiza/expertiza 
===Link to Forked Repository===
https://github.com/adityajoshi1114/expertiza 
===Link to Pull Request===
https://github.com/expertiza/expertiza/pull/2663

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-12-05T03:00:27Z

Hrmauny: /* Project tracking */

==Problem Statement==
Our project concentrates on making things readable and understandable in <code>app/helpers/review_mapping_helper.rb</code> file. We have have refactored some methods mentioned below for better understanding while maintaining the functionality. The aim is to also make it better understandable for other developers by adding comments wherever necessary.

==Team==
===Mentor===
Devashish Vachhani (dvachha@ncsu.edu)

===Team Members===
Harsh Mauny (hrmauny@ncsu.edu) 
Connor Davidson (cdavids@ncsu.edu) 
Aditya Joshi (ajoshi25@ncsu.edu) 

==Issues to be Addressed==

* '''Issue #1:'''The next several methods generate charts. They are cohesive enough that they should be in their own separate file, either another helper or a mixin.

* '''Issue #2:'''Various method names begin with get. This is not Ruby-like. Change the names to something more appropriate.

* '''Issue #3:'''There are several methods for feedback_response_maps. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.

* '''Issue #4:'''The method sort_reviewer_by_review_volume_desc should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #6:'''The file ends with three small classes being defined. There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

==Plan of work==
We started our work by analysing the <code>review_mapping_helper.rb</code> how it links to rest of the code and discussing the flow of the project in detail with our mentor. Based on our understanding of the code we divided our project requirements in terms of difficulty levels as shown in the [https://docs.google.com/document/d/1AmXVfhH3icn8Pc9oJ3U7okER2NhBDIFFtFxTofmrKcE/edit?usp=sharing document]. then we distributed the tasks among us and started refactoring various methods and creating new files as and when necessary. We also ensured the test cases passing earlier were intact and added new ones if necessary.

=='''Project 4 - DESIGN DOC'''==
Following are the changes we have been assigned to do after completion of project 3

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow
Apart from these issues we have added some issues of our own based on the feedback of project 3 review which are as follows

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

* A lot of variables are difficult to understand and hence we plan to refactor the variable for better readability and understanding
* We were not able to increase the code coverage after our changes during project 3. Here in project 4 we want to focus on writing more tests and using the test skeleton.

Below is an UML diagram already uploaded in Expertiza database documentation for reader reference 
[[File:Review mappings imported.png|700px]] 

====Design Patterns====
The Code present in review_mapping_helper can be improved by using design patterns to improve maintainability, readability, and flexibility.
Following are some design patterns that are suggested

* Methods can be extracted into smaller, well maintained with just a single responsibility. This will promote code readability and promote code reuse.

*The methods review_report_data, team_color, obtain_team_color, and others follow a common sequence of steps but allow for variation in some steps. Applying the Template Method Pattern can help define a common structure in a base method while allowing specific steps to be implemented in subclasses or overridden in derived methods.

*Descriptive and short naming conventions are required so the methods and variables are self explanatory.

* Separation of Concerns should be followed for following code.

====Work Plan====

The plan of work on the above issues is as follows 

*'''Issue #5:''' There is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
'''Methods involved:''' <code> list_review_submissions </code> 
'''Problem:''' following are the problems here as stated by the task 
* Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
[[File:E2353-issue-5-orignal-issue.png|700px]] 
'''Solution:''' Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why previous students implemented this method
* Reading the code and the usages of this method to understand how it's being used
* better variable and method names if needed.
 

*'''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow 
'''Methods involved:''' <code> get_data_for_review_report </code> 
'''Problem:''' following are the problems here as stated by the task 
* There are several methods for feedback_response_maps.
* It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing. 
[[File:E2353-issue-7-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why this method was originally implemented
* Reading the code and the usages of this method to understand how it's being used
* refactor to make better variable and method names if necessary.
 

*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green. 
[[File:E2353-issue-8-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The colour word has to be replaced by color for consistency
* In order to improve the readability we can reduce the cognitive complexity and branching by using less if-else block and further dividing the method into multiple block.
* More comments can be added to make sure the reader understands what each line does in these methods.
* make sure proper color get assigned based on the review status by making sure all tests pass after refactoring and maybe new test cases can be added
* there are some unwanted array data structure used in order to pass the color code which should be removed to reduce complexity.
* better variable and method names if needed.
 

* '''Issue #9:''' get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.
'''Methods involved:''' <code>get_awarded_review_score</code> 
'''Problem:''' following are the problems here as stated by the task 
* The method computes an overall score based upon scores awarded in individual rounds, but score calculation is being standardized now in response_map.rb and the method needs to be modified to use the new code. 
[[File:E2353-issue-9-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The score calculation code in response_map.rb needs to be understood thoroughly and as stated, has to be used in the <code>get_awarded_review_score</code> method
* We plan to contact Nicholas to get further clarity in our understanding of the code before we proceed to use it
* We will add comments as necessary once the standardized code has been utilized in the review mapping helper so that readers can understand what the code fragments are doing
* The current variable names will be improved
 

==Files Modified==

* app/helpers/charts_helper.rb
* app/helpers/review_mapping_helper.rb
* app/helpers/review_mapping_helper.rb
* app/views/reports/_calibration_report.html.erb
* app/views/reports/_feedback_report.html.erb
* app/views/reports/_review_report.html.erb
* app/views/reports/_team_score.html.erb
* app/views/reports/_team_score_score_awarded.html.erb
* spec/features/review_mapping_helper_spec.rb
* spec/helpers/review_mapping_helper_spec.rb

==Project tracking==
please use this template
*'''Issue #[number]:''' text 
'''Methods involved:''' <code>text</code> 
'''Problem:''' text 
'''Solution:'''text, code, images if any 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Name
 

'''This section contains combined changes of project 3 and 4'''
----

* '''Issue #1:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
'''Methods involved:''' <code>initialize_chart_elements()</code>, <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code>, <code>calculate_key_chart_information()</code> 
'''Previous Code:''' All these methods were previously a part of the <code>review_mapping_helper.rb</code> when it should have its separate helper file 
'''Problem:''' These methods perform similar tasks of generating graphs and can have there separate module or a mixin and also each of the methods more than 30 lines inside them making them too large 
'''Solution:''' distributed this code among 2 newly created files named <code>review_mapping_charts_helper.rb</code> and <code>data_mapping_helper.rb</code> within helper folder. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. <code>review_mapping_charts_helper.rb</code> contains the original methods and <code>data_mapping_helper.rb</code> manages the data need by these chart methods 

'''review_mapping_charts_helper.rb'''
<pre>
module ReviewMappingChartsHelper
...
def initialize_chart_elements()
def display_volume_metric_chart()
def display_tagging_interval_chart()
def calculate_key_chart_information()
</pre>
 
'''all link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #2:''' change methods names starting with "get" to something appropriate 
'''Methods involved:''' <code> get_data_for_review_report()</code>, <code> get_team_color()</code>, <code> get_link_updated_at()</code>, <code> get_team_reviewed_link_name()</code>, <code> get_awarded_review_score()</code>, <code> get_each_review_and_feedback_response_map()</code>, <code> get_certain_review_and_feedback_response_map()</code>, <code> get_css_style_for_calibration_report()</code> 
'''Previous Code:''' previous code used "get" in method names 
'''Problem:''' All these methods used "get_" which is not ruby-like. 
'''Solution:''' change method names to a more ruby-like name that was appropriate for the given context 

'''Contributor:''' Connor Davidson
 
----
*'''Issue #3:''' Several methods for feedback_response_maps, check docs to manage this confusion 
'''Methods involved:''' <code> feedback_response_for_author()</code> 
'''Previous Code:''' previous code had an ambiguous variable name 
'''Problem:''' It wasn't clear what the variable was used for. 
'''Solution:''' refactor the variable name to be more easily understood and add comments to give a clear explanation for why it exists 

'''Contributor:''' Connor Davidson
 
----

*'''Issue #4:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
'''Problem:''' Currently the method sort_reviewer_by_review_volume_desc sorts the reviewers in descending order but only considers one metric which is review volume which makes it very specific and it cannot be used if we want to sort by other metrics 
'''Solution:'''The solution here is make this a generalized method by renaming <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and accepting a metric as argument on which we can sort the the reviewers as shown below. Since it was used in <code>review_report</code> partial the implementation was changed there as well 
'''review_mapping_helper.rb''' 
[[File: E2353-Issue-4-1.png|900px]] 
'''app/views/reports/_review_report.html.erb''' 
[[File:E2353issue-4-2.png|900px]] 
'''review_mapping_helper_spec.rb''' 
[[File:E2353-issue4-3.png|900px]] 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-1f05f505eaa5edaf0790cdeb6e774cc499685cce4edf12b253ecd358ce710582 review_mapping_helper_spec.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #5:''' The method <code>list_review_submissions</code> is not clear in terms of its utility
'''Methods involved:''' <code>list_review_submissions</code> 
'''Problem:''' It is not clear what the method does, where its used and if it can be replaced. 
'''Solution:''' Comments (shown below) have been added to explain the usage of the method both at the method call in the partial <code>_review_report.html.erb</code> and the method definition in <code>review_mapping_helper.rb</code>

<code>review_mapping_helper</code> code snippet
<pre>
# Extracts the files submitted for a particular pair of participant and reviewee
def list_review_submissions(participant_id, reviewee_team_id, response_map_id)
participant = Participant.find(participant_id)
team = AssignmentTeam.find(reviewee_team_id)
html = ''
unless team.nil? || participant.nil?
# Build a path to the review submissions using the team's path and the response map ID
review_submissions_path = team.path + '_review' + '/' + response_map_id.to_s
files = team.submitted_files(review_submissions_path)
html += display_review_files_directory_tree(participant, files) if files.present?
</pre>

<code>_review_report</code> code snippet
<pre>
# Following Table entry displays review submissions for specific reviewer and reviewee
<%= list_review_submissions(reviewer.id, reviewer_map.reviewee_id, reviewer_map.id) %>


<%= list_hyperlink_submission(reviewer_map.id, 5386) if Assignment.find_by(id: @id.to_i).try(:course).try(:instructor).try(:name) == 'Jkidd'%>
</div>
</pre>

'''Justification for keeping the method:''' 
-> The method has been added to the helper to reduce the complexity of logic in the views which is inherently good programming practice 
-> It builds the parameters required to feed into <code>display_review_files_directory_tree</code> method - a piece of logic that otherwise would have reduced the readability of the code in the partial 
-> Although the helper is just used at one place, its necessary to generate review report tables and hence seems to be the appropriate solution in terms of both functionality and code readability 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
'''Contributor:''' Aditya Joshi
 
----

*'''Issue #6:''' The Last 3 Classes in review_helper need to be reviewed 
'''Methods involved:''' <code>initialize</code>, <code>reviews_needed</code>, <code>reviews_per_team</code>, <code>reviews_per_student</code> (separate methods for both the sub classes) 
'''Problem:''' The file ends with three small classes being defined. There are no comments at all to explain what is being done 
'''Solution:'''The usage of the three classes was traced back to assignment views and ReviewMappingController. It is evident that these classes have been used to encapsulate the logic for different review strategies in the system. Some comments have been added to explain the functionality of these classes. 
'''review_strategy classes''' 
[[File:E2353-Issue-6.png|900px]] 
'''Further explanation of the methods:''' 
<code>reviews_needed</code> : Calculates the total number of reviews needed for all teams/participants 
<code>reviews_per_team</code> : Calculates the number of reviews each team should receive 
<code>reviews_per_student</code> (for StudentReviewStrategy) : Specifies the number of reviews each student should perform 
<code>reviews_per_student</code> (for TeamReviewStrategy) : Specifies the number of reviews each student should perform based on team assignments 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Aditya Joshi
 
----
*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green.
Below is the original code
<pre>
def team_color(response_map)
# Storing redundantly computed value in a variable
assignment_created = @assignment.created_at
# Storing redundantly computed value in a variable
assignment_due_dates = DueDate.where(parent_id: response_map.reviewed_object_id)
# Returning color based on conditions
if Response.exists?(map_id: response_map.id)
if !response_map.try(:reviewer).try(:review_grade).nil?
'brown'
elsif response_for_each_round?(response_map)
'blue'
else
obtain_team_color(response_map, assignment_created, assignment_due_dates)
end
else
'red'
end
end

# loops through the number of assignment review rounds and obtains the team color
def obtain_team_color(response_map, assignment_created, assignment_due_dates)
color = []
(1..@assignment.num_review_rounds).each do |round|
check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
end
color[-1]
end

# checks the submission state within each round and assigns team color
def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
if submitted_within_round?(round, response_map, assignment_created, assignment_due_dates)
color.push 'purple'
else
link = submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates)
if link.nil? || (link !~ %r{https*:\/\/wiki(.*)}) # can be extended for github links in future
color.push 'green'
else
link_updated_at = get_link_updated_at(link)
color.push link_updated_since_last?(round, assignment_due_dates, link_updated_at) ? 'purple' : 'green'
end
end
end
</pre>
 

'''Solution:''' First we understood the color coding scheme based on the references provided to us and based on which the code was refactored to address the problem. Below are the changes that were made
* The colour word was replaced by color for consistency
* In order to improve the readability the cognitive complexity and branching was reduced by using less if-else block and further dividing the method into multiple block.
* comments were added providing an explanation of what the code does
* making sure proper color get assigned based on the review status
* better variable and method names wherever needed. 
[[File:E2353-issue-8-updated-team-color.png|700px]] 
'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
----

==Test Plan==
This is a refactoring project, so it is expected that the original functionality should be retained. Some of the task involved adding comments to previous code, changing variable name etc. which no change was required in test cases. Some methods where method names were changed or they were shifted to other files, for these methods necessary changes were made in respective Rspec files. All the test cases passed which implies that the refactored code did not break the existing functionality.
 

After Project 3 we have changed some method and variable names to improve readability of <code>review_mapping_helper.rb</code> and relevant file and in turn made appropriate changes in respective test files to make sure all test cases are passing. Some new test cases have been added as suggested by test skeleton files in <code>review_mapping_helper_spec.rb</code> example of some of the test cases are below 
<pre>
describe 'test calculate_key_chart_information' do
it 'should return new Hash if intervals are not empty' do
intervals = [1.00, 2.00, 3.00, 4.00, 5.00, 6.00]
result = helper.calculate_key_chart_information(intervals)
expect(result).to be_a_kind_of(Hash)
expect(result[:mean]).to eq(3.50)
expect(result[:min]).to eq(1.00)
expect(result[:max]).to eq(6.00)
expect(result[:variance]).to eq(2.92)
expect(result[:stand_dev]).to eq(1.71)
end
it 'returns the mean, min, max, variance, and standard deviation of the intervals' do
expect(calculate_key_chart_information([10, 15, 20])).to eq(mean: 15.0, min: 10, max: 20, variance: 16.67, stand_dev: 4.08)
expect(calculate_key_chart_information([5, 8, 12, 15, 20])).to eq(mean: 12.0, min: 5, max: 20, variance: 27.6, stand_dev: 5.25)
end
context 'when intervals are empty' do
it 'returns an empty hash' do
# Test case 4
expect(calculate_key_chart_information([])).to eq({})
end
end
context 'when intervals contain values greater than the threshold' do
it 'ignores those intervals and returns an empty hash' do
expect(calculate_key_chart_information([60, 45, 35])).to eq({})
end
end
end
</pre>

<pre>
describe 'test calibration_report_css_class' do
context 'when the difference is 0' do
it 'returns c5 as the CSS class' do
css0 = helper.calibration_report_css_class(0)
expect(css0). to eq('c5')
end
end
context 'when the difference is 1' do
it 'returns c4 as the CSS class' do
css1 = helper.calibration_report_css_class(-1)
expect(css1). to eq('c4')
end
end
context 'when the difference is 2' do
it 'returns c3 as the CSS class' do
css2 = helper.calibration_report_css_class(-2)
expect(css2). to eq('c3')
end
end
context 'when the difference is 3' do
it 'returns c2 as the CSS class' do
css3 = helper.calibration_report_css_class(-3)
expect(css3). to eq('c2')
end
end
context 'when the difference is greater than 3' do
it 'returns c1 as the CSS class' do
css4 = helper.calibration_report_css_class(6)
expect(css4). to eq('c1')
end
end
end
</pre>
 The image below shows the test cases are passing as well as most of the code climate issues were resolved. 
[[File:E2353-Test-Report.png|250px]]

==Github Links==
===Link to Expertiza Repository===
https://github.com/expertiza/expertiza 
===Link to Forked Repository===
https://github.com/adityajoshi1114/expertiza 
===Link to Pull Request===
https://github.com/expertiza/expertiza/pull/2663

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-12-05T02:59:08Z

Hrmauny: /* Test Plan */

==Problem Statement==
Our project concentrates on making things readable and understandable in <code>app/helpers/review_mapping_helper.rb</code> file. We have have refactored some methods mentioned below for better understanding while maintaining the functionality. The aim is to also make it better understandable for other developers by adding comments wherever necessary.

==Team==
===Mentor===
Devashish Vachhani (dvachha@ncsu.edu)

===Team Members===
Harsh Mauny (hrmauny@ncsu.edu) 
Connor Davidson (cdavids@ncsu.edu) 
Aditya Joshi (ajoshi25@ncsu.edu) 

==Issues to be Addressed==

* '''Issue #1:'''The next several methods generate charts. They are cohesive enough that they should be in their own separate file, either another helper or a mixin.

* '''Issue #2:'''Various method names begin with get. This is not Ruby-like. Change the names to something more appropriate.

* '''Issue #3:'''There are several methods for feedback_response_maps. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.

* '''Issue #4:'''The method sort_reviewer_by_review_volume_desc should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #6:'''The file ends with three small classes being defined. There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

==Plan of work==
We started our work by analysing the <code>review_mapping_helper.rb</code> how it links to rest of the code and discussing the flow of the project in detail with our mentor. Based on our understanding of the code we divided our project requirements in terms of difficulty levels as shown in the [https://docs.google.com/document/d/1AmXVfhH3icn8Pc9oJ3U7okER2NhBDIFFtFxTofmrKcE/edit?usp=sharing document]. then we distributed the tasks among us and started refactoring various methods and creating new files as and when necessary. We also ensured the test cases passing earlier were intact and added new ones if necessary.

=='''Project 4 - DESIGN DOC'''==
Following are the changes we have been assigned to do after completion of project 3

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow
Apart from these issues we have added some issues of our own based on the feedback of project 3 review which are as follows

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

* A lot of variables are difficult to understand and hence we plan to refactor the variable for better readability and understanding
* We were not able to increase the code coverage after our changes during project 3. Here in project 4 we want to focus on writing more tests and using the test skeleton.

Below is an UML diagram already uploaded in Expertiza database documentation for reader reference 
[[File:Review mappings imported.png|700px]] 

====Design Patterns====
The Code present in review_mapping_helper can be improved by using design patterns to improve maintainability, readability, and flexibility.
Following are some design patterns that are suggested

* Methods can be extracted into smaller, well maintained with just a single responsibility. This will promote code readability and promote code reuse.

*The methods review_report_data, team_color, obtain_team_color, and others follow a common sequence of steps but allow for variation in some steps. Applying the Template Method Pattern can help define a common structure in a base method while allowing specific steps to be implemented in subclasses or overridden in derived methods.

*Descriptive and short naming conventions are required so the methods and variables are self explanatory.

* Separation of Concerns should be followed for following code.

====Work Plan====

The plan of work on the above issues is as follows 

*'''Issue #5:''' There is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
'''Methods involved:''' <code> list_review_submissions </code> 
'''Problem:''' following are the problems here as stated by the task 
* Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
[[File:E2353-issue-5-orignal-issue.png|700px]] 
'''Solution:''' Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why previous students implemented this method
* Reading the code and the usages of this method to understand how it's being used
* better variable and method names if needed.
 

*'''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow 
'''Methods involved:''' <code> get_data_for_review_report </code> 
'''Problem:''' following are the problems here as stated by the task 
* There are several methods for feedback_response_maps.
* It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing. 
[[File:E2353-issue-7-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why this method was originally implemented
* Reading the code and the usages of this method to understand how it's being used
* refactor to make better variable and method names if necessary.
 

*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green. 
[[File:E2353-issue-8-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The colour word has to be replaced by color for consistency
* In order to improve the readability we can reduce the cognitive complexity and branching by using less if-else block and further dividing the method into multiple block.
* More comments can be added to make sure the reader understands what each line does in these methods.
* make sure proper color get assigned based on the review status by making sure all tests pass after refactoring and maybe new test cases can be added
* there are some unwanted array data structure used in order to pass the color code which should be removed to reduce complexity.
* better variable and method names if needed.
 

* '''Issue #9:''' get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.
'''Methods involved:''' <code>get_awarded_review_score</code> 
'''Problem:''' following are the problems here as stated by the task 
* The method computes an overall score based upon scores awarded in individual rounds, but score calculation is being standardized now in response_map.rb and the method needs to be modified to use the new code. 
[[File:E2353-issue-9-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The score calculation code in response_map.rb needs to be understood thoroughly and as stated, has to be used in the <code>get_awarded_review_score</code> method
* We plan to contact Nicholas to get further clarity in our understanding of the code before we proceed to use it
* We will add comments as necessary once the standardized code has been utilized in the review mapping helper so that readers can understand what the code fragments are doing
* The current variable names will be improved
 

==Files Modified==

* app/helpers/charts_helper.rb
* app/helpers/review_mapping_helper.rb
* app/helpers/review_mapping_helper.rb
* app/views/reports/_calibration_report.html.erb
* app/views/reports/_feedback_report.html.erb
* app/views/reports/_review_report.html.erb
* app/views/reports/_team_score.html.erb
* app/views/reports/_team_score_score_awarded.html.erb
* spec/features/review_mapping_helper_spec.rb
* spec/helpers/review_mapping_helper_spec.rb

==Project tracking==
please use this template
*'''Issue #[number]:''' text 
'''Methods involved:''' <code>text</code> 
'''Problem:''' text 
'''Solution:'''text, code, images if any 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Name
 

----

* '''Issue #1:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
'''Methods involved:''' <code>initialize_chart_elements()</code>, <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code>, <code>calculate_key_chart_information()</code> 
'''Previous Code:''' All these methods were previously a part of the <code>review_mapping_helper.rb</code> when it should have its separate helper file 
'''Problem:''' These methods perform similar tasks of generating graphs and can have there separate module or a mixin and also each of the methods more than 30 lines inside them making them too large 
'''Solution:''' distributed this code among 2 newly created files named <code>review_mapping_charts_helper.rb</code> and <code>data_mapping_helper.rb</code> within helper folder. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. <code>review_mapping_charts_helper.rb</code> contains the original methods and <code>data_mapping_helper.rb</code> manages the data need by these chart methods 

'''review_mapping_charts_helper.rb'''
<pre>
module ReviewMappingChartsHelper
...
def initialize_chart_elements()
def display_volume_metric_chart()
def display_tagging_interval_chart()
def calculate_key_chart_information()
</pre>
 
'''all link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #2:''' change methods names starting with "get" to something appropriate 
'''Methods involved:''' <code> get_data_for_review_report()</code>, <code> get_team_color()</code>, <code> get_link_updated_at()</code>, <code> get_team_reviewed_link_name()</code>, <code> get_awarded_review_score()</code>, <code> get_each_review_and_feedback_response_map()</code>, <code> get_certain_review_and_feedback_response_map()</code>, <code> get_css_style_for_calibration_report()</code> 
'''Previous Code:''' previous code used "get" in method names 
'''Problem:''' All these methods used "get_" which is not ruby-like. 
'''Solution:''' change method names to a more ruby-like name that was appropriate for the given context 

'''Contributor:''' Connor Davidson
 
----
*'''Issue #3:''' Several methods for feedback_response_maps, check docs to manage this confusion 
'''Methods involved:''' <code> feedback_response_for_author()</code> 
'''Previous Code:''' previous code had an ambiguous variable name 
'''Problem:''' It wasn't clear what the variable was used for. 
'''Solution:''' refactor the variable name to be more easily understood and add comments to give a clear explanation for why it exists 

'''Contributor:''' Connor Davidson
 
----

*'''Issue #4:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
'''Problem:''' Currently the method sort_reviewer_by_review_volume_desc sorts the reviewers in descending order but only considers one metric which is review volume which makes it very specific and it cannot be used if we want to sort by other metrics 
'''Solution:'''The solution here is make this a generalized method by renaming <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and accepting a metric as argument on which we can sort the the reviewers as shown below. Since it was used in <code>review_report</code> partial the implementation was changed there as well 
'''review_mapping_helper.rb''' 
[[File: E2353-Issue-4-1.png|900px]] 
'''app/views/reports/_review_report.html.erb''' 
[[File:E2353issue-4-2.png|900px]] 
'''review_mapping_helper_spec.rb''' 
[[File:E2353-issue4-3.png|900px]] 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-1f05f505eaa5edaf0790cdeb6e774cc499685cce4edf12b253ecd358ce710582 review_mapping_helper_spec.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #5:''' The method <code>list_review_submissions</code> is not clear in terms of its utility
'''Methods involved:''' <code>list_review_submissions</code> 
'''Problem:''' It is not clear what the method does, where its used and if it can be replaced. 
'''Solution:''' Comments (shown below) have been added to explain the usage of the method both at the method call in the partial <code>_review_report.html.erb</code> and the method definition in <code>review_mapping_helper.rb</code>

<code>review_mapping_helper</code> code snippet
<pre>
# Extracts the files submitted for a particular pair of participant and reviewee
def list_review_submissions(participant_id, reviewee_team_id, response_map_id)
participant = Participant.find(participant_id)
team = AssignmentTeam.find(reviewee_team_id)
html = ''
unless team.nil? || participant.nil?
# Build a path to the review submissions using the team's path and the response map ID
review_submissions_path = team.path + '_review' + '/' + response_map_id.to_s
files = team.submitted_files(review_submissions_path)
html += display_review_files_directory_tree(participant, files) if files.present?
</pre>

<code>_review_report</code> code snippet
<pre>
# Following Table entry displays review submissions for specific reviewer and reviewee
<%= list_review_submissions(reviewer.id, reviewer_map.reviewee_id, reviewer_map.id) %>


<%= list_hyperlink_submission(reviewer_map.id, 5386) if Assignment.find_by(id: @id.to_i).try(:course).try(:instructor).try(:name) == 'Jkidd'%>
</div>
</pre>

'''Justification for keeping the method:''' 
-> The method has been added to the helper to reduce the complexity of logic in the views which is inherently good programming practice 
-> It builds the parameters required to feed into <code>display_review_files_directory_tree</code> method - a piece of logic that otherwise would have reduced the readability of the code in the partial 
-> Although the helper is just used at one place, its necessary to generate review report tables and hence seems to be the appropriate solution in terms of both functionality and code readability 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
'''Contributor:''' Aditya Joshi
 
----

*'''Issue #6:''' The Last 3 Classes in review_helper need to be reviewed 
'''Methods involved:''' <code>initialize</code>, <code>reviews_needed</code>, <code>reviews_per_team</code>, <code>reviews_per_student</code> (separate methods for both the sub classes) 
'''Problem:''' The file ends with three small classes being defined. There are no comments at all to explain what is being done 
'''Solution:'''The usage of the three classes was traced back to assignment views and ReviewMappingController. It is evident that these classes have been used to encapsulate the logic for different review strategies in the system. Some comments have been added to explain the functionality of these classes. 
'''review_strategy classes''' 
[[File:E2353-Issue-6.png|900px]] 
'''Further explanation of the methods:''' 
<code>reviews_needed</code> : Calculates the total number of reviews needed for all teams/participants 
<code>reviews_per_team</code> : Calculates the number of reviews each team should receive 
<code>reviews_per_student</code> (for StudentReviewStrategy) : Specifies the number of reviews each student should perform 
<code>reviews_per_student</code> (for TeamReviewStrategy) : Specifies the number of reviews each student should perform based on team assignments 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Aditya Joshi
 
----
*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green.
Below is the original code
<pre>
def team_color(response_map)
# Storing redundantly computed value in a variable
assignment_created = @assignment.created_at
# Storing redundantly computed value in a variable
assignment_due_dates = DueDate.where(parent_id: response_map.reviewed_object_id)
# Returning color based on conditions
if Response.exists?(map_id: response_map.id)
if !response_map.try(:reviewer).try(:review_grade).nil?
'brown'
elsif response_for_each_round?(response_map)
'blue'
else
obtain_team_color(response_map, assignment_created, assignment_due_dates)
end
else
'red'
end
end

# loops through the number of assignment review rounds and obtains the team color
def obtain_team_color(response_map, assignment_created, assignment_due_dates)
color = []
(1..@assignment.num_review_rounds).each do |round|
check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
end
color[-1]
end

# checks the submission state within each round and assigns team color
def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
if submitted_within_round?(round, response_map, assignment_created, assignment_due_dates)
color.push 'purple'
else
link = submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates)
if link.nil? || (link !~ %r{https*:\/\/wiki(.*)}) # can be extended for github links in future
color.push 'green'
else
link_updated_at = get_link_updated_at(link)
color.push link_updated_since_last?(round, assignment_due_dates, link_updated_at) ? 'purple' : 'green'
end
end
end
</pre>
 

'''Solution:''' First we understood the color coding scheme based on the references provided to us and based on which the code was refactored to address the problem. Below are the changes that were made
* The colour word was replaced by color for consistency
* In order to improve the readability the cognitive complexity and branching was reduced by using less if-else block and further dividing the method into multiple block.
* comments were added providing an explanation of what the code does
* making sure proper color get assigned based on the review status
* better variable and method names wherever needed. 
[[File:E2353-issue-8-updated-team-color.png|700px]] 
'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
----
==Test Plan==
This is a refactoring project, so it is expected that the original functionality should be retained. Some of the task involved adding comments to previous code, changing variable name etc. which no change was required in test cases. Some methods where method names were changed or they were shifted to other files, for these methods necessary changes were made in respective Rspec files. All the test cases passed which implies that the refactored code did not break the existing functionality.
 

After Project 3 we have changed some method and variable names to improve readability of <code>review_mapping_helper.rb</code> and relevant file and in turn made appropriate changes in respective test files to make sure all test cases are passing. Some new test cases have been added as suggested by test skeleton files in <code>review_mapping_helper_spec.rb</code> example of some of the test cases are below 
<pre>
describe 'test calculate_key_chart_information' do
it 'should return new Hash if intervals are not empty' do
intervals = [1.00, 2.00, 3.00, 4.00, 5.00, 6.00]
result = helper.calculate_key_chart_information(intervals)
expect(result).to be_a_kind_of(Hash)
expect(result[:mean]).to eq(3.50)
expect(result[:min]).to eq(1.00)
expect(result[:max]).to eq(6.00)
expect(result[:variance]).to eq(2.92)
expect(result[:stand_dev]).to eq(1.71)
end
it 'returns the mean, min, max, variance, and standard deviation of the intervals' do
expect(calculate_key_chart_information([10, 15, 20])).to eq(mean: 15.0, min: 10, max: 20, variance: 16.67, stand_dev: 4.08)
expect(calculate_key_chart_information([5, 8, 12, 15, 20])).to eq(mean: 12.0, min: 5, max: 20, variance: 27.6, stand_dev: 5.25)
end
context 'when intervals are empty' do
it 'returns an empty hash' do
# Test case 4
expect(calculate_key_chart_information([])).to eq({})
end
end
context 'when intervals contain values greater than the threshold' do
it 'ignores those intervals and returns an empty hash' do
expect(calculate_key_chart_information([60, 45, 35])).to eq({})
end
end
end
</pre>

<pre>
describe 'test calibration_report_css_class' do
context 'when the difference is 0' do
it 'returns c5 as the CSS class' do
css0 = helper.calibration_report_css_class(0)
expect(css0). to eq('c5')
end
end
context 'when the difference is 1' do
it 'returns c4 as the CSS class' do
css1 = helper.calibration_report_css_class(-1)
expect(css1). to eq('c4')
end
end
context 'when the difference is 2' do
it 'returns c3 as the CSS class' do
css2 = helper.calibration_report_css_class(-2)
expect(css2). to eq('c3')
end
end
context 'when the difference is 3' do
it 'returns c2 as the CSS class' do
css3 = helper.calibration_report_css_class(-3)
expect(css3). to eq('c2')
end
end
context 'when the difference is greater than 3' do
it 'returns c1 as the CSS class' do
css4 = helper.calibration_report_css_class(6)
expect(css4). to eq('c1')
end
end
end
</pre>
 The image below shows the test cases are passing as well as most of the code climate issues were resolved. 
[[File:E2353-Test-Report.png|250px]]

==Github Links==
===Link to Expertiza Repository===
https://github.com/expertiza/expertiza 
===Link to Forked Repository===
https://github.com/adityajoshi1114/expertiza 
===Link to Pull Request===
https://github.com/expertiza/expertiza/pull/2663

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-12-05T02:57:46Z

Hrmauny: /* Test Plan */

==Problem Statement==
Our project concentrates on making things readable and understandable in <code>app/helpers/review_mapping_helper.rb</code> file. We have have refactored some methods mentioned below for better understanding while maintaining the functionality. The aim is to also make it better understandable for other developers by adding comments wherever necessary.

==Team==
===Mentor===
Devashish Vachhani (dvachha@ncsu.edu)

===Team Members===
Harsh Mauny (hrmauny@ncsu.edu) 
Connor Davidson (cdavids@ncsu.edu) 
Aditya Joshi (ajoshi25@ncsu.edu) 

==Issues to be Addressed==

* '''Issue #1:'''The next several methods generate charts. They are cohesive enough that they should be in their own separate file, either another helper or a mixin.

* '''Issue #2:'''Various method names begin with get. This is not Ruby-like. Change the names to something more appropriate.

* '''Issue #3:'''There are several methods for feedback_response_maps. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.

* '''Issue #4:'''The method sort_reviewer_by_review_volume_desc should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #6:'''The file ends with three small classes being defined. There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

==Plan of work==
We started our work by analysing the <code>review_mapping_helper.rb</code> how it links to rest of the code and discussing the flow of the project in detail with our mentor. Based on our understanding of the code we divided our project requirements in terms of difficulty levels as shown in the [https://docs.google.com/document/d/1AmXVfhH3icn8Pc9oJ3U7okER2NhBDIFFtFxTofmrKcE/edit?usp=sharing document]. then we distributed the tasks among us and started refactoring various methods and creating new files as and when necessary. We also ensured the test cases passing earlier were intact and added new ones if necessary.

=='''Project 4 - DESIGN DOC'''==
Following are the changes we have been assigned to do after completion of project 3

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow
Apart from these issues we have added some issues of our own based on the feedback of project 3 review which are as follows

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

* A lot of variables are difficult to understand and hence we plan to refactor the variable for better readability and understanding
* We were not able to increase the code coverage after our changes during project 3. Here in project 4 we want to focus on writing more tests and using the test skeleton.

Below is an UML diagram already uploaded in Expertiza database documentation for reader reference 
[[File:Review mappings imported.png|700px]] 

====Design Patterns====
The Code present in review_mapping_helper can be improved by using design patterns to improve maintainability, readability, and flexibility.
Following are some design patterns that are suggested

* Methods can be extracted into smaller, well maintained with just a single responsibility. This will promote code readability and promote code reuse.

*The methods review_report_data, team_color, obtain_team_color, and others follow a common sequence of steps but allow for variation in some steps. Applying the Template Method Pattern can help define a common structure in a base method while allowing specific steps to be implemented in subclasses or overridden in derived methods.

*Descriptive and short naming conventions are required so the methods and variables are self explanatory.

* Separation of Concerns should be followed for following code.

====Work Plan====

The plan of work on the above issues is as follows 

*'''Issue #5:''' There is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
'''Methods involved:''' <code> list_review_submissions </code> 
'''Problem:''' following are the problems here as stated by the task 
* Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
[[File:E2353-issue-5-orignal-issue.png|700px]] 
'''Solution:''' Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why previous students implemented this method
* Reading the code and the usages of this method to understand how it's being used
* better variable and method names if needed.
 

*'''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow 
'''Methods involved:''' <code> get_data_for_review_report </code> 
'''Problem:''' following are the problems here as stated by the task 
* There are several methods for feedback_response_maps.
* It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing. 
[[File:E2353-issue-7-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why this method was originally implemented
* Reading the code and the usages of this method to understand how it's being used
* refactor to make better variable and method names if necessary.
 

*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green. 
[[File:E2353-issue-8-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The colour word has to be replaced by color for consistency
* In order to improve the readability we can reduce the cognitive complexity and branching by using less if-else block and further dividing the method into multiple block.
* More comments can be added to make sure the reader understands what each line does in these methods.
* make sure proper color get assigned based on the review status by making sure all tests pass after refactoring and maybe new test cases can be added
* there are some unwanted array data structure used in order to pass the color code which should be removed to reduce complexity.
* better variable and method names if needed.
 

* '''Issue #9:''' get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.
'''Methods involved:''' <code>get_awarded_review_score</code> 
'''Problem:''' following are the problems here as stated by the task 
* The method computes an overall score based upon scores awarded in individual rounds, but score calculation is being standardized now in response_map.rb and the method needs to be modified to use the new code. 
[[File:E2353-issue-9-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The score calculation code in response_map.rb needs to be understood thoroughly and as stated, has to be used in the <code>get_awarded_review_score</code> method
* We plan to contact Nicholas to get further clarity in our understanding of the code before we proceed to use it
* We will add comments as necessary once the standardized code has been utilized in the review mapping helper so that readers can understand what the code fragments are doing
* The current variable names will be improved
 

==Files Modified==

* app/helpers/charts_helper.rb
* app/helpers/review_mapping_helper.rb
* app/helpers/review_mapping_helper.rb
* app/views/reports/_calibration_report.html.erb
* app/views/reports/_feedback_report.html.erb
* app/views/reports/_review_report.html.erb
* app/views/reports/_team_score.html.erb
* app/views/reports/_team_score_score_awarded.html.erb
* spec/features/review_mapping_helper_spec.rb
* spec/helpers/review_mapping_helper_spec.rb

==Project tracking==
please use this template
*'''Issue #[number]:''' text 
'''Methods involved:''' <code>text</code> 
'''Problem:''' text 
'''Solution:'''text, code, images if any 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Name
 

----

* '''Issue #1:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
'''Methods involved:''' <code>initialize_chart_elements()</code>, <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code>, <code>calculate_key_chart_information()</code> 
'''Previous Code:''' All these methods were previously a part of the <code>review_mapping_helper.rb</code> when it should have its separate helper file 
'''Problem:''' These methods perform similar tasks of generating graphs and can have there separate module or a mixin and also each of the methods more than 30 lines inside them making them too large 
'''Solution:''' distributed this code among 2 newly created files named <code>review_mapping_charts_helper.rb</code> and <code>data_mapping_helper.rb</code> within helper folder. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. <code>review_mapping_charts_helper.rb</code> contains the original methods and <code>data_mapping_helper.rb</code> manages the data need by these chart methods 

'''review_mapping_charts_helper.rb'''
<pre>
module ReviewMappingChartsHelper
...
def initialize_chart_elements()
def display_volume_metric_chart()
def display_tagging_interval_chart()
def calculate_key_chart_information()
</pre>
 
'''all link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #2:''' change methods names starting with "get" to something appropriate 
'''Methods involved:''' <code> get_data_for_review_report()</code>, <code> get_team_color()</code>, <code> get_link_updated_at()</code>, <code> get_team_reviewed_link_name()</code>, <code> get_awarded_review_score()</code>, <code> get_each_review_and_feedback_response_map()</code>, <code> get_certain_review_and_feedback_response_map()</code>, <code> get_css_style_for_calibration_report()</code> 
'''Previous Code:''' previous code used "get" in method names 
'''Problem:''' All these methods used "get_" which is not ruby-like. 
'''Solution:''' change method names to a more ruby-like name that was appropriate for the given context 

'''Contributor:''' Connor Davidson
 
----
*'''Issue #3:''' Several methods for feedback_response_maps, check docs to manage this confusion 
'''Methods involved:''' <code> feedback_response_for_author()</code> 
'''Previous Code:''' previous code had an ambiguous variable name 
'''Problem:''' It wasn't clear what the variable was used for. 
'''Solution:''' refactor the variable name to be more easily understood and add comments to give a clear explanation for why it exists 

'''Contributor:''' Connor Davidson
 
----

*'''Issue #4:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
'''Problem:''' Currently the method sort_reviewer_by_review_volume_desc sorts the reviewers in descending order but only considers one metric which is review volume which makes it very specific and it cannot be used if we want to sort by other metrics 
'''Solution:'''The solution here is make this a generalized method by renaming <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and accepting a metric as argument on which we can sort the the reviewers as shown below. Since it was used in <code>review_report</code> partial the implementation was changed there as well 
'''review_mapping_helper.rb''' 
[[File: E2353-Issue-4-1.png|900px]] 
'''app/views/reports/_review_report.html.erb''' 
[[File:E2353issue-4-2.png|900px]] 
'''review_mapping_helper_spec.rb''' 
[[File:E2353-issue4-3.png|900px]] 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-1f05f505eaa5edaf0790cdeb6e774cc499685cce4edf12b253ecd358ce710582 review_mapping_helper_spec.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #5:''' The method <code>list_review_submissions</code> is not clear in terms of its utility
'''Methods involved:''' <code>list_review_submissions</code> 
'''Problem:''' It is not clear what the method does, where its used and if it can be replaced. 
'''Solution:''' Comments (shown below) have been added to explain the usage of the method both at the method call in the partial <code>_review_report.html.erb</code> and the method definition in <code>review_mapping_helper.rb</code>

<code>review_mapping_helper</code> code snippet
<pre>
# Extracts the files submitted for a particular pair of participant and reviewee
def list_review_submissions(participant_id, reviewee_team_id, response_map_id)
participant = Participant.find(participant_id)
team = AssignmentTeam.find(reviewee_team_id)
html = ''
unless team.nil? || participant.nil?
# Build a path to the review submissions using the team's path and the response map ID
review_submissions_path = team.path + '_review' + '/' + response_map_id.to_s
files = team.submitted_files(review_submissions_path)
html += display_review_files_directory_tree(participant, files) if files.present?
</pre>

<code>_review_report</code> code snippet
<pre>
# Following Table entry displays review submissions for specific reviewer and reviewee
<%= list_review_submissions(reviewer.id, reviewer_map.reviewee_id, reviewer_map.id) %>


<%= list_hyperlink_submission(reviewer_map.id, 5386) if Assignment.find_by(id: @id.to_i).try(:course).try(:instructor).try(:name) == 'Jkidd'%>
</div>
</pre>

'''Justification for keeping the method:''' 
-> The method has been added to the helper to reduce the complexity of logic in the views which is inherently good programming practice 
-> It builds the parameters required to feed into <code>display_review_files_directory_tree</code> method - a piece of logic that otherwise would have reduced the readability of the code in the partial 
-> Although the helper is just used at one place, its necessary to generate review report tables and hence seems to be the appropriate solution in terms of both functionality and code readability 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
'''Contributor:''' Aditya Joshi
 
----

*'''Issue #6:''' The Last 3 Classes in review_helper need to be reviewed 
'''Methods involved:''' <code>initialize</code>, <code>reviews_needed</code>, <code>reviews_per_team</code>, <code>reviews_per_student</code> (separate methods for both the sub classes) 
'''Problem:''' The file ends with three small classes being defined. There are no comments at all to explain what is being done 
'''Solution:'''The usage of the three classes was traced back to assignment views and ReviewMappingController. It is evident that these classes have been used to encapsulate the logic for different review strategies in the system. Some comments have been added to explain the functionality of these classes. 
'''review_strategy classes''' 
[[File:E2353-Issue-6.png|900px]] 
'''Further explanation of the methods:''' 
<code>reviews_needed</code> : Calculates the total number of reviews needed for all teams/participants 
<code>reviews_per_team</code> : Calculates the number of reviews each team should receive 
<code>reviews_per_student</code> (for StudentReviewStrategy) : Specifies the number of reviews each student should perform 
<code>reviews_per_student</code> (for TeamReviewStrategy) : Specifies the number of reviews each student should perform based on team assignments 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Aditya Joshi
 
----
*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green.
Below is the original code
<pre>
def team_color(response_map)
# Storing redundantly computed value in a variable
assignment_created = @assignment.created_at
# Storing redundantly computed value in a variable
assignment_due_dates = DueDate.where(parent_id: response_map.reviewed_object_id)
# Returning color based on conditions
if Response.exists?(map_id: response_map.id)
if !response_map.try(:reviewer).try(:review_grade).nil?
'brown'
elsif response_for_each_round?(response_map)
'blue'
else
obtain_team_color(response_map, assignment_created, assignment_due_dates)
end
else
'red'
end
end

# loops through the number of assignment review rounds and obtains the team color
def obtain_team_color(response_map, assignment_created, assignment_due_dates)
color = []
(1..@assignment.num_review_rounds).each do |round|
check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
end
color[-1]
end

# checks the submission state within each round and assigns team color
def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
if submitted_within_round?(round, response_map, assignment_created, assignment_due_dates)
color.push 'purple'
else
link = submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates)
if link.nil? || (link !~ %r{https*:\/\/wiki(.*)}) # can be extended for github links in future
color.push 'green'
else
link_updated_at = get_link_updated_at(link)
color.push link_updated_since_last?(round, assignment_due_dates, link_updated_at) ? 'purple' : 'green'
end
end
end
</pre>
 

'''Solution:''' First we understood the color coding scheme based on the references provided to us and based on which the code was refactored to address the problem. Below are the changes that were made
* The colour word was replaced by color for consistency
* In order to improve the readability the cognitive complexity and branching was reduced by using less if-else block and further dividing the method into multiple block.
* comments were added providing an explanation of what the code does
* making sure proper color get assigned based on the review status
* better variable and method names wherever needed. 
[[File:E2353-issue-8-updated-team-color.png|700px]] 
'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
----
==Test Plan==
This is a refactoring project, so it is expected that the original functionality should be retained. Some of the task involved adding comments to previous code, changing variable name etc. which no change was required in test cases. Some methods where method names were changed or they were shifted to other files, for these methods necessary changes were made in respective Rspec files. All the test cases passed which implies that the refactored code did not break the existing functionality.
 

After Project 3 we have changed some method and variable names to improve readability of <code>review_mapping_helper.rb</code> and relevant file and in turn made appropriate changes in respective test files to make sure all test cases are passing. Some new test cases have been added as suggested by test skeleton files in <code>review_mapping_helper_spec.rb</code> example of some of the test cases are below 
<code>
describe 'test calculate_key_chart_information' do
it 'should return new Hash if intervals are not empty' do
intervals = [1.00, 2.00, 3.00, 4.00, 5.00, 6.00]
result = helper.calculate_key_chart_information(intervals)
expect(result).to be_a_kind_of(Hash)
expect(result[:mean]).to eq(3.50)
expect(result[:min]).to eq(1.00)
expect(result[:max]).to eq(6.00)
expect(result[:variance]).to eq(2.92)
expect(result[:stand_dev]).to eq(1.71)
end
it 'returns the mean, min, max, variance, and standard deviation of the intervals' do
expect(calculate_key_chart_information([10, 15, 20])).to eq(mean: 15.0, min: 10, max: 20, variance: 16.67, stand_dev: 4.08)
expect(calculate_key_chart_information([5, 8, 12, 15, 20])).to eq(mean: 12.0, min: 5, max: 20, variance: 27.6, stand_dev: 5.25)
end
context 'when intervals are empty' do
it 'returns an empty hash' do
# Test case 4
expect(calculate_key_chart_information([])).to eq({})
end
end
context 'when intervals contain values greater than the threshold' do
it 'ignores those intervals and returns an empty hash' do
expect(calculate_key_chart_information([60, 45, 35])).to eq({})
end
end
end
</code>

<code>
describe 'test calibration_report_css_class' do
context 'when the difference is 0' do
it 'returns c5 as the CSS class' do
css0 = helper.calibration_report_css_class(0)
expect(css0). to eq('c5')
end
end
context 'when the difference is 1' do
it 'returns c4 as the CSS class' do
css1 = helper.calibration_report_css_class(-1)
expect(css1). to eq('c4')
end
end
context 'when the difference is 2' do
it 'returns c3 as the CSS class' do
css2 = helper.calibration_report_css_class(-2)
expect(css2). to eq('c3')
end
end
context 'when the difference is 3' do
it 'returns c2 as the CSS class' do
css3 = helper.calibration_report_css_class(-3)
expect(css3). to eq('c2')
end
end
context 'when the difference is greater than 3' do
it 'returns c1 as the CSS class' do
css4 = helper.calibration_report_css_class(6)
expect(css4). to eq('c1')
end
end
end
</code>
 The image below shows the test cases are passing as well as most of the code climate issues were resolved. 
[[File:E2353-Test-Report.png|250px]]

==Github Links==
===Link to Expertiza Repository===
https://github.com/expertiza/expertiza 
===Link to Forked Repository===
https://github.com/adityajoshi1114/expertiza 
===Link to Pull Request===
https://github.com/expertiza/expertiza/pull/2663

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-12-05T02:05:36Z

Hrmauny: /* Test Plan */

==Problem Statement==
Our project concentrates on making things readable and understandable in <code>app/helpers/review_mapping_helper.rb</code> file. We have have refactored some methods mentioned below for better understanding while maintaining the functionality. The aim is to also make it better understandable for other developers by adding comments wherever necessary.

==Team==
===Mentor===
Devashish Vachhani (dvachha@ncsu.edu)

===Team Members===
Harsh Mauny (hrmauny@ncsu.edu) 
Connor Davidson (cdavids@ncsu.edu) 
Aditya Joshi (ajoshi25@ncsu.edu) 

==Issues to be Addressed==

* '''Issue #1:'''The next several methods generate charts. They are cohesive enough that they should be in their own separate file, either another helper or a mixin.

* '''Issue #2:'''Various method names begin with get. This is not Ruby-like. Change the names to something more appropriate.

* '''Issue #3:'''There are several methods for feedback_response_maps. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.

* '''Issue #4:'''The method sort_reviewer_by_review_volume_desc should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #6:'''The file ends with three small classes being defined. There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

==Plan of work==
We started our work by analysing the <code>review_mapping_helper.rb</code> how it links to rest of the code and discussing the flow of the project in detail with our mentor. Based on our understanding of the code we divided our project requirements in terms of difficulty levels as shown in the [https://docs.google.com/document/d/1AmXVfhH3icn8Pc9oJ3U7okER2NhBDIFFtFxTofmrKcE/edit?usp=sharing document]. then we distributed the tasks among us and started refactoring various methods and creating new files as and when necessary. We also ensured the test cases passing earlier were intact and added new ones if necessary.

=='''Project 4 - DESIGN DOC'''==
Following are the changes we have been assigned to do after completion of project 3

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

* '''Issue #10:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow
Apart from these issues we have added some issues of our own based on the feedback of project 3 review which are as follows

* A lot of variables are difficult to understand and hence we plan to refactor the variable for better readability and understanding
* We were not able to increase the code coverage after our changes during project 3. Here in project 4 we want to focus on writing more tests and using the test skeleton.

Below is an UML diagram already uploaded in Expertiza database documentation for reader reference 
[[File:Review mappings imported.png|700px]] 

====Design Patterns====
The Code present in review_mapping_helper can be improved by using design patterns to improve maintainability, readability, and flexibility.
Following are some design patterns that are suggested

* Methods can be extracted into smaller, well maintained with just a single responsibility. This will promote code readability and promote code reuse.

*The methods review_report_data, team_color, obtain_team_color, and others follow a common sequence of steps but allow for variation in some steps. Applying the Template Method Pattern can help define a common structure in a base method while allowing specific steps to be implemented in subclasses or overridden in derived methods.

*Descriptive and short naming conventions are required so the methods and variables are self explanatory.

* Separation of Concerns should be followed for following code.

====Work Plan====

The plan of work on the above issues is as follows 

*'''Issue #5:''' There is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
'''Methods involved:''' <code> list_review_submissions </code> 
'''Problem:''' following are the problems here as stated by the task 
* Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
[[File:E2353-issue-5-orignal-issue.png|700px]] 
'''Solution:''' Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why previous students implemented this method
* Reading the code and the usages of this method to understand how it's being used
* better variable and method names if needed.
 

*'''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow 
'''Methods involved:''' <code> get_data_for_review_report </code> 
'''Problem:''' following are the problems here as stated by the task 
* There are several methods for feedback_response_maps.
* It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing. 
[[File:E2353-issue-7-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why this method was originally implemented
* Reading the code and the usages of this method to understand how it's being used
* refactor to make better variable and method names if necessary.
 

*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green. 
[[File:E2353-issue-8-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The colour word has to be replaced by color for consistency
* In order to improve the readability we can reduce the cognitive complexity and branching by using less if-else block and further dividing the method into multiple block.
* More comments can be added to make sure the reader understands what each line does in these methods.
* make sure proper color get assigned based on the review status by making sure all tests pass after refactoring and maybe new test cases can be added
* there are some unwanted array data structure used in order to pass the color code which should be removed to reduce complexity.
* better variable and method names if needed.
 

* '''Issue #9:''' get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.
'''Methods involved:''' <code>get_awarded_review_score</code> 
'''Problem:''' following are the problems here as stated by the task 
* The method computes an overall score based upon scores awarded in individual rounds, but score calculation is being standardized now in response_map.rb and the method needs to be modified to use the new code. 
[[File:E2353-issue-9-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The score calculation code in response_map.rb needs to be understood thoroughly and as stated, has to be used in the <code>get_awarded_review_score</code> method
* We plan to contact Nicholas to get further clarity in our understanding of the code before we proceed to use it
* We will add comments as necessary once the standardized code has been utilized in the review mapping helper so that readers can understand what the code fragments are doing
* The current variable names will be improved
 

==Files Modified==

* app/helpers/charts_helper.rb
* app/helpers/review_mapping_helper.rb
* app/helpers/review_mapping_helper.rb
* app/views/reports/_calibration_report.html.erb
* app/views/reports/_feedback_report.html.erb
* app/views/reports/_review_report.html.erb
* app/views/reports/_team_score.html.erb
* app/views/reports/_team_score_score_awarded.html.erb
* spec/features/review_mapping_helper_spec.rb
* spec/helpers/review_mapping_helper_spec.rb

==Project tracking==
please use this template
*'''Issue #[number]:''' text 
'''Methods involved:''' <code>text</code> 
'''Problem:''' text 
'''Solution:'''text, code, images if any 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Name
 

----

* '''Issue #1:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
'''Methods involved:''' <code>initialize_chart_elements()</code>, <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code>, <code>calculate_key_chart_information()</code> 
'''Previous Code:''' All these methods were previously a part of the <code>review_mapping_helper.rb</code> when it should have its separate helper file 
'''Problem:''' These methods perform similar tasks of generating graphs and can have there separate module or a mixin and also each of the methods more than 30 lines inside them making them too large 
'''Solution:''' distributed this code among 2 newly created files named <code>review_mapping_charts_helper.rb</code> and <code>data_mapping_helper.rb</code> within helper folder. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. <code>review_mapping_charts_helper.rb</code> contains the original methods and <code>data_mapping_helper.rb</code> manages the data need by these chart methods 

'''review_mapping_charts_helper.rb'''
<pre>
module ReviewMappingChartsHelper
...
def initialize_chart_elements()
def display_volume_metric_chart()
def display_tagging_interval_chart()
def calculate_key_chart_information()
</pre>
 
'''all link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #2:''' change methods names starting with "get" to something appropriate 
'''Methods involved:''' <code> get_data_for_review_report()</code>, <code> get_team_color()</code>, <code> get_link_updated_at()</code>, <code> get_team_reviewed_link_name()</code>, <code> get_awarded_review_score()</code>, <code> get_each_review_and_feedback_response_map()</code>, <code> get_certain_review_and_feedback_response_map()</code>, <code> get_css_style_for_calibration_report()</code> 
'''Previous Code:''' previous code used "get" in method names 
'''Problem:''' All these methods used "get_" which is not ruby-like. 
'''Solution:''' change method names to a more ruby-like name that was appropriate for the given context 

'''Contributor:''' Connor Davidson
 
----
*'''Issue #3:''' Several methods for feedback_response_maps, check docs to manage this confusion 
'''Methods involved:''' <code> feedback_response_for_author()</code> 
'''Previous Code:''' previous code had an ambiguous variable name 
'''Problem:''' It wasn't clear what the variable was used for. 
'''Solution:''' refactor the variable name to be more easily understood and add comments to give a clear explanation for why it exists 

'''Contributor:''' Connor Davidson
 
----

*'''Issue #4:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
'''Problem:''' Currently the method sort_reviewer_by_review_volume_desc sorts the reviewers in descending order but only considers one metric which is review volume which makes it very specific and it cannot be used if we want to sort by other metrics 
'''Solution:'''The solution here is make this a generalized method by renaming <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and accepting a metric as argument on which we can sort the the reviewers as shown below. Since it was used in <code>review_report</code> partial the implementation was changed there as well 
'''review_mapping_helper.rb''' 
[[File: E2353-Issue-4-1.png|900px]] 
'''app/views/reports/_review_report.html.erb''' 
[[File:E2353issue-4-2.png|900px]] 
'''review_mapping_helper_spec.rb''' 
[[File:E2353-issue4-3.png|900px]] 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-1f05f505eaa5edaf0790cdeb6e774cc499685cce4edf12b253ecd358ce710582 review_mapping_helper_spec.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #5:''' The method <code>list_review_submissions</code> is not clear in terms of its utility
'''Methods involved:''' <code>list_review_submissions</code> 
'''Problem:''' It is not clear what the method does, where its used and if it can be replaced. 
'''Solution:''' Comments (shown below) have been added to explain the usage of the method both at the method call in the partial <code>_review_report.html.erb</code> and the method definition in <code>review_mapping_helper.rb</code>

<code>review_mapping_helper</code> code snippet
<pre>
# Extracts the files submitted for a particular pair of participant and reviewee
def list_review_submissions(participant_id, reviewee_team_id, response_map_id)
participant = Participant.find(participant_id)
team = AssignmentTeam.find(reviewee_team_id)
html = ''
unless team.nil? || participant.nil?
# Build a path to the review submissions using the team's path and the response map ID
review_submissions_path = team.path + '_review' + '/' + response_map_id.to_s
files = team.submitted_files(review_submissions_path)
html += display_review_files_directory_tree(participant, files) if files.present?
</pre>

<code>_review_report</code> code snippet
<pre>
# Following Table entry displays review submissions for specific reviewer and reviewee
<%= list_review_submissions(reviewer.id, reviewer_map.reviewee_id, reviewer_map.id) %>


<%= list_hyperlink_submission(reviewer_map.id, 5386) if Assignment.find_by(id: @id.to_i).try(:course).try(:instructor).try(:name) == 'Jkidd'%>
</div>
</pre>

'''Justification for keeping the method:''' 
-> The method has been added to the helper to reduce the complexity of logic in the views which is inherently good programming practice 
-> It builds the parameters required to feed into <code>display_review_files_directory_tree</code> method - a piece of logic that otherwise would have reduced the readability of the code in the partial 
-> Although the helper is just used at one place, its necessary to generate review report tables and hence seems to be the appropriate solution in terms of both functionality and code readability 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
'''Contributor:''' Aditya Joshi
 
----

*'''Issue #6:''' The Last 3 Classes in review_helper need to be reviewed 
'''Methods involved:''' <code>initialize</code>, <code>reviews_needed</code>, <code>reviews_per_team</code>, <code>reviews_per_student</code> (separate methods for both the sub classes) 
'''Problem:''' The file ends with three small classes being defined. There are no comments at all to explain what is being done 
'''Solution:'''The usage of the three classes was traced back to assignment views and ReviewMappingController. It is evident that these classes have been used to encapsulate the logic for different review strategies in the system. Some comments have been added to explain the functionality of these classes. 
'''review_strategy classes''' 
[[File:E2353-Issue-6.png|900px]] 
'''Further explanation of the methods:''' 
<code>reviews_needed</code> : Calculates the total number of reviews needed for all teams/participants 
<code>reviews_per_team</code> : Calculates the number of reviews each team should receive 
<code>reviews_per_student</code> (for StudentReviewStrategy) : Specifies the number of reviews each student should perform 
<code>reviews_per_student</code> (for TeamReviewStrategy) : Specifies the number of reviews each student should perform based on team assignments 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Aditya Joshi
 
----
*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green.
Below is the original code
<pre>
def team_color(response_map)
# Storing redundantly computed value in a variable
assignment_created = @assignment.created_at
# Storing redundantly computed value in a variable
assignment_due_dates = DueDate.where(parent_id: response_map.reviewed_object_id)
# Returning color based on conditions
if Response.exists?(map_id: response_map.id)
if !response_map.try(:reviewer).try(:review_grade).nil?
'brown'
elsif response_for_each_round?(response_map)
'blue'
else
obtain_team_color(response_map, assignment_created, assignment_due_dates)
end
else
'red'
end
end

# loops through the number of assignment review rounds and obtains the team color
def obtain_team_color(response_map, assignment_created, assignment_due_dates)
color = []
(1..@assignment.num_review_rounds).each do |round|
check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
end
color[-1]
end

# checks the submission state within each round and assigns team color
def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
if submitted_within_round?(round, response_map, assignment_created, assignment_due_dates)
color.push 'purple'
else
link = submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates)
if link.nil? || (link !~ %r{https*:\/\/wiki(.*)}) # can be extended for github links in future
color.push 'green'
else
link_updated_at = get_link_updated_at(link)
color.push link_updated_since_last?(round, assignment_due_dates, link_updated_at) ? 'purple' : 'green'
end
end
end
</pre>
 

'''Solution:''' First we understood the color coding scheme based on the references provided to us and based on which the code was refactored to address the problem. Below are the changes that were made
* The colour word was replaced by color for consistency
* In order to improve the readability the cognitive complexity and branching was reduced by using less if-else block and further dividing the method into multiple block.
* comments were added providing an explanation of what the code does
* making sure proper color get assigned based on the review status
* better variable and method names wherever needed. 
[[File:E2353-issue-8-updated-team-color.png|700px]] 
'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
----
==Test Plan==
This is a refactoring project, so it is expected that the original functionality should be retained. Some of the task involved adding comments to previous code, changing variable name etc. which no change was required in test cases. Some methods where method names were changed or they were shifted to other files, for these methods necessary changes were made in respective Rspec files. All the test cases passed which implies that the refactored code did not break the existing functionality.
 

After Project 3 we have changed some method and variable names to improve readability of <code>review_mapping_helper.rb</code> and relevant file and in turn made appropriate changes in respective test files to make sure all test cases are passing. Some new test cases have been added as suggested by test skeleton files in <code>review_mapping_helper_spec.rb</code> example of some of the test cases are below 
<code>
describe 'test calculate_key_chart_information' do
it 'should return new Hash if intervals are not empty' do
intervals = [1.00, 2.00, 3.00, 4.00, 5.00, 6.00]
result = helper.calculate_key_chart_information(intervals)
expect(result).to be_a_kind_of(Hash)
expect(result[:mean]).to eq(3.50)
expect(result[:min]).to eq(1.00)
expect(result[:max]).to eq(6.00)
expect(result[:variance]).to eq(2.92)
expect(result[:stand_dev]).to eq(1.71)
end
it 'returns the mean, min, max, variance, and standard deviation of the intervals' do
expect(calculate_key_chart_information([10, 15, 20])).to eq(mean: 15.0, min: 10, max: 20, variance: 16.67, stand_dev: 4.08)
expect(calculate_key_chart_information([5, 8, 12, 15, 20])).to eq(mean: 12.0, min: 5, max: 20, variance: 27.6, stand_dev: 5.25)
end
context 'when intervals are empty' do
it 'returns an empty hash' do
# Test case 4
expect(calculate_key_chart_information([])).to eq({})
end
end
context 'when intervals contain values greater than the threshold' do
it 'ignores those intervals and returns an empty hash' do
expect(calculate_key_chart_information([60, 45, 35])).to eq({})
end
end
end
</code> 

<code>
describe 'test calibration_report_css_class' do
context 'when the difference is 0' do
it 'returns c5 as the CSS class' do
css0 = helper.calibration_report_css_class(0)
expect(css0). to eq('c5')
end
end
context 'when the difference is 1' do
it 'returns c4 as the CSS class' do
css1 = helper.calibration_report_css_class(-1)
expect(css1). to eq('c4')
end
end
context 'when the difference is 2' do
it 'returns c3 as the CSS class' do
css2 = helper.calibration_report_css_class(-2)
expect(css2). to eq('c3')
end
end
context 'when the difference is 3' do
it 'returns c2 as the CSS class' do
css3 = helper.calibration_report_css_class(-3)
expect(css3). to eq('c2')
end
end
context 'when the difference is greater than 3' do
it 'returns c1 as the CSS class' do
css4 = helper.calibration_report_css_class(6)
expect(css4). to eq('c1')
end
end
end
</code> 
The image below shows the test cases are passing as well as most of the code climate issues were resolved. 
[[File:E2353-Test-Report.png|250px]]

==Github Links==
===Link to Expertiza Repository===
https://github.com/expertiza/expertiza 
===Link to Forked Repository===
https://github.com/adityajoshi1114/expertiza 
===Link to Pull Request===
https://github.com/expertiza/expertiza/pull/2663

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-12-05T01:53:47Z

Hrmauny: /* Test Plan */

==Problem Statement==
Our project concentrates on making things readable and understandable in <code>app/helpers/review_mapping_helper.rb</code> file. We have have refactored some methods mentioned below for better understanding while maintaining the functionality. The aim is to also make it better understandable for other developers by adding comments wherever necessary.

==Team==
===Mentor===
Devashish Vachhani (dvachha@ncsu.edu)

===Team Members===
Harsh Mauny (hrmauny@ncsu.edu) 
Connor Davidson (cdavids@ncsu.edu) 
Aditya Joshi (ajoshi25@ncsu.edu) 

==Issues to be Addressed==

* '''Issue #1:'''The next several methods generate charts. They are cohesive enough that they should be in their own separate file, either another helper or a mixin.

* '''Issue #2:'''Various method names begin with get. This is not Ruby-like. Change the names to something more appropriate.

* '''Issue #3:'''There are several methods for feedback_response_maps. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.

* '''Issue #4:'''The method sort_reviewer_by_review_volume_desc should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #6:'''The file ends with three small classes being defined. There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

==Plan of work==
We started our work by analysing the <code>review_mapping_helper.rb</code> how it links to rest of the code and discussing the flow of the project in detail with our mentor. Based on our understanding of the code we divided our project requirements in terms of difficulty levels as shown in the [https://docs.google.com/document/d/1AmXVfhH3icn8Pc9oJ3U7okER2NhBDIFFtFxTofmrKcE/edit?usp=sharing document]. then we distributed the tasks among us and started refactoring various methods and creating new files as and when necessary. We also ensured the test cases passing earlier were intact and added new ones if necessary.

=='''Project 4 - DESIGN DOC'''==
Following are the changes we have been assigned to do after completion of project 3

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

* '''Issue #10:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow
Apart from these issues we have added some issues of our own based on the feedback of project 3 review which are as follows

* A lot of variables are difficult to understand and hence we plan to refactor the variable for better readability and understanding
* We were not able to increase the code coverage after our changes during project 3. Here in project 4 we want to focus on writing more tests and using the test skeleton.

Below is an UML diagram already uploaded in Expertiza database documentation for reader reference 
[[File:Review mappings imported.png|700px]] 

====Design Patterns====
The Code present in review_mapping_helper can be improved by using design patterns to improve maintainability, readability, and flexibility.
Following are some design patterns that are suggested

* Methods can be extracted into smaller, well maintained with just a single responsibility. This will promote code readability and promote code reuse.

*The methods review_report_data, team_color, obtain_team_color, and others follow a common sequence of steps but allow for variation in some steps. Applying the Template Method Pattern can help define a common structure in a base method while allowing specific steps to be implemented in subclasses or overridden in derived methods.

*Descriptive and short naming conventions are required so the methods and variables are self explanatory.

* Separation of Concerns should be followed for following code.

====Work Plan====

The plan of work on the above issues is as follows 

*'''Issue #5:''' There is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
'''Methods involved:''' <code> list_review_submissions </code> 
'''Problem:''' following are the problems here as stated by the task 
* Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
[[File:E2353-issue-5-orignal-issue.png|700px]] 
'''Solution:''' Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why previous students implemented this method
* Reading the code and the usages of this method to understand how it's being used
* better variable and method names if needed.
 

*'''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow 
'''Methods involved:''' <code> get_data_for_review_report </code> 
'''Problem:''' following are the problems here as stated by the task 
* There are several methods for feedback_response_maps.
* It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing. 
[[File:E2353-issue-7-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why this method was originally implemented
* Reading the code and the usages of this method to understand how it's being used
* refactor to make better variable and method names if necessary.
 

*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green. 
[[File:E2353-issue-8-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The colour word has to be replaced by color for consistency
* In order to improve the readability we can reduce the cognitive complexity and branching by using less if-else block and further dividing the method into multiple block.
* More comments can be added to make sure the reader understands what each line does in these methods.
* make sure proper color get assigned based on the review status by making sure all tests pass after refactoring and maybe new test cases can be added
* there are some unwanted array data structure used in order to pass the color code which should be removed to reduce complexity.
* better variable and method names if needed.
 

* '''Issue #9:''' get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.
'''Methods involved:''' <code>get_awarded_review_score</code> 
'''Problem:''' following are the problems here as stated by the task 
* The method computes an overall score based upon scores awarded in individual rounds, but score calculation is being standardized now in response_map.rb and the method needs to be modified to use the new code. 
[[File:E2353-issue-9-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The score calculation code in response_map.rb needs to be understood thoroughly and as stated, has to be used in the <code>get_awarded_review_score</code> method
* We plan to contact Nicholas to get further clarity in our understanding of the code before we proceed to use it
* We will add comments as necessary once the standardized code has been utilized in the review mapping helper so that readers can understand what the code fragments are doing
* The current variable names will be improved
 

==Files Modified==

* app/helpers/charts_helper.rb
* app/helpers/review_mapping_helper.rb
* app/helpers/review_mapping_helper.rb
* app/views/reports/_calibration_report.html.erb
* app/views/reports/_feedback_report.html.erb
* app/views/reports/_review_report.html.erb
* app/views/reports/_team_score.html.erb
* app/views/reports/_team_score_score_awarded.html.erb
* spec/features/review_mapping_helper_spec.rb
* spec/helpers/review_mapping_helper_spec.rb

==Project tracking==
please use this template
*'''Issue #[number]:''' text 
'''Methods involved:''' <code>text</code> 
'''Problem:''' text 
'''Solution:'''text, code, images if any 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Name
 

----

* '''Issue #1:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
'''Methods involved:''' <code>initialize_chart_elements()</code>, <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code>, <code>calculate_key_chart_information()</code> 
'''Previous Code:''' All these methods were previously a part of the <code>review_mapping_helper.rb</code> when it should have its separate helper file 
'''Problem:''' These methods perform similar tasks of generating graphs and can have there separate module or a mixin and also each of the methods more than 30 lines inside them making them too large 
'''Solution:''' distributed this code among 2 newly created files named <code>review_mapping_charts_helper.rb</code> and <code>data_mapping_helper.rb</code> within helper folder. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. <code>review_mapping_charts_helper.rb</code> contains the original methods and <code>data_mapping_helper.rb</code> manages the data need by these chart methods 

'''review_mapping_charts_helper.rb'''
<pre>
module ReviewMappingChartsHelper
...
def initialize_chart_elements()
def display_volume_metric_chart()
def display_tagging_interval_chart()
def calculate_key_chart_information()
</pre>
 
'''all link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #2:''' change methods names starting with "get" to something appropriate 
'''Methods involved:''' <code> get_data_for_review_report()</code>, <code> get_team_color()</code>, <code> get_link_updated_at()</code>, <code> get_team_reviewed_link_name()</code>, <code> get_awarded_review_score()</code>, <code> get_each_review_and_feedback_response_map()</code>, <code> get_certain_review_and_feedback_response_map()</code>, <code> get_css_style_for_calibration_report()</code> 
'''Previous Code:''' previous code used "get" in method names 
'''Problem:''' All these methods used "get_" which is not ruby-like. 
'''Solution:''' change method names to a more ruby-like name that was appropriate for the given context 

'''Contributor:''' Connor Davidson
 
----
*'''Issue #3:''' Several methods for feedback_response_maps, check docs to manage this confusion 
'''Methods involved:''' <code> feedback_response_for_author()</code> 
'''Previous Code:''' previous code had an ambiguous variable name 
'''Problem:''' It wasn't clear what the variable was used for. 
'''Solution:''' refactor the variable name to be more easily understood and add comments to give a clear explanation for why it exists 

'''Contributor:''' Connor Davidson
 
----

*'''Issue #4:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
'''Problem:''' Currently the method sort_reviewer_by_review_volume_desc sorts the reviewers in descending order but only considers one metric which is review volume which makes it very specific and it cannot be used if we want to sort by other metrics 
'''Solution:'''The solution here is make this a generalized method by renaming <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and accepting a metric as argument on which we can sort the the reviewers as shown below. Since it was used in <code>review_report</code> partial the implementation was changed there as well 
'''review_mapping_helper.rb''' 
[[File: E2353-Issue-4-1.png|900px]] 
'''app/views/reports/_review_report.html.erb''' 
[[File:E2353issue-4-2.png|900px]] 
'''review_mapping_helper_spec.rb''' 
[[File:E2353-issue4-3.png|900px]] 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-1f05f505eaa5edaf0790cdeb6e774cc499685cce4edf12b253ecd358ce710582 review_mapping_helper_spec.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #5:''' The method <code>list_review_submissions</code> is not clear in terms of its utility
'''Methods involved:''' <code>list_review_submissions</code> 
'''Problem:''' It is not clear what the method does, where its used and if it can be replaced. 
'''Solution:''' Comments (shown below) have been added to explain the usage of the method both at the method call in the partial <code>_review_report.html.erb</code> and the method definition in <code>review_mapping_helper.rb</code>

<code>review_mapping_helper</code> code snippet
<pre>
# Extracts the files submitted for a particular pair of participant and reviewee
def list_review_submissions(participant_id, reviewee_team_id, response_map_id)
participant = Participant.find(participant_id)
team = AssignmentTeam.find(reviewee_team_id)
html = ''
unless team.nil? || participant.nil?
# Build a path to the review submissions using the team's path and the response map ID
review_submissions_path = team.path + '_review' + '/' + response_map_id.to_s
files = team.submitted_files(review_submissions_path)
html += display_review_files_directory_tree(participant, files) if files.present?
</pre>

<code>_review_report</code> code snippet
<pre>
# Following Table entry displays review submissions for specific reviewer and reviewee
<%= list_review_submissions(reviewer.id, reviewer_map.reviewee_id, reviewer_map.id) %>


<%= list_hyperlink_submission(reviewer_map.id, 5386) if Assignment.find_by(id: @id.to_i).try(:course).try(:instructor).try(:name) == 'Jkidd'%>
</div>
</pre>

'''Justification for keeping the method:''' 
-> The method has been added to the helper to reduce the complexity of logic in the views which is inherently good programming practice 
-> It builds the parameters required to feed into <code>display_review_files_directory_tree</code> method - a piece of logic that otherwise would have reduced the readability of the code in the partial 
-> Although the helper is just used at one place, its necessary to generate review report tables and hence seems to be the appropriate solution in terms of both functionality and code readability 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
'''Contributor:''' Aditya Joshi
 
----

*'''Issue #6:''' The Last 3 Classes in review_helper need to be reviewed 
'''Methods involved:''' <code>initialize</code>, <code>reviews_needed</code>, <code>reviews_per_team</code>, <code>reviews_per_student</code> (separate methods for both the sub classes) 
'''Problem:''' The file ends with three small classes being defined. There are no comments at all to explain what is being done 
'''Solution:'''The usage of the three classes was traced back to assignment views and ReviewMappingController. It is evident that these classes have been used to encapsulate the logic for different review strategies in the system. Some comments have been added to explain the functionality of these classes. 
'''review_strategy classes''' 
[[File:E2353-Issue-6.png|900px]] 
'''Further explanation of the methods:''' 
<code>reviews_needed</code> : Calculates the total number of reviews needed for all teams/participants 
<code>reviews_per_team</code> : Calculates the number of reviews each team should receive 
<code>reviews_per_student</code> (for StudentReviewStrategy) : Specifies the number of reviews each student should perform 
<code>reviews_per_student</code> (for TeamReviewStrategy) : Specifies the number of reviews each student should perform based on team assignments 

'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Aditya Joshi
 
----
*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green.
Below is the original code
<pre>
def team_color(response_map)
# Storing redundantly computed value in a variable
assignment_created = @assignment.created_at
# Storing redundantly computed value in a variable
assignment_due_dates = DueDate.where(parent_id: response_map.reviewed_object_id)
# Returning color based on conditions
if Response.exists?(map_id: response_map.id)
if !response_map.try(:reviewer).try(:review_grade).nil?
'brown'
elsif response_for_each_round?(response_map)
'blue'
else
obtain_team_color(response_map, assignment_created, assignment_due_dates)
end
else
'red'
end
end

# loops through the number of assignment review rounds and obtains the team color
def obtain_team_color(response_map, assignment_created, assignment_due_dates)
color = []
(1..@assignment.num_review_rounds).each do |round|
check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
end
color[-1]
end

# checks the submission state within each round and assigns team color
def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
if submitted_within_round?(round, response_map, assignment_created, assignment_due_dates)
color.push 'purple'
else
link = submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates)
if link.nil? || (link !~ %r{https*:\/\/wiki(.*)}) # can be extended for github links in future
color.push 'green'
else
link_updated_at = get_link_updated_at(link)
color.push link_updated_since_last?(round, assignment_due_dates, link_updated_at) ? 'purple' : 'green'
end
end
end
</pre>
 

'''Solution:''' First we understood the color coding scheme based on the references provided to us and based on which the code was refactored to address the problem. Below are the changes that were made
* The colour word was replaced by color for consistency
* In order to improve the readability the cognitive complexity and branching was reduced by using less if-else block and further dividing the method into multiple block.
* comments were added providing an explanation of what the code does
* making sure proper color get assigned based on the review status
* better variable and method names wherever needed. 
[[File:E2353-issue-8-updated-team-color.png|700px]] 
'''All links related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
----
==Test Plan==
This is a refactoring project, so it is expected that the original functionality should be retained. Some of the task involved adding comments to previous code, changing variable name etc. which no change was required in test cases. Some methods where method names were changed or they were shifted to other files, for these methods necessary changes were made in respective Rspec files. All the test cases passed which implies that the refactored code did not break the existing functionality.
 

After Project 3 we have changed some method and variable names to improve readability of review_mapper_helper.rb and relevant file and made appropriate changes in respective test files to make sure all test cases are passing. Some new test cases have been added in review_mapping_helper_spec.rb example of some of the test cases are below
[[File:E2353-Test-Report.png|250px]]

==Github Links==
===Link to Expertiza Repository===
https://github.com/expertiza/expertiza 
===Link to Forked Repository===
https://github.com/adityajoshi1114/expertiza 
===Link to Pull Request===
https://github.com/expertiza/expertiza/pull/2663

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-25T06:11:55Z

Hrmauny:

==Problem Statement==
Our project concentrates on making things readable and understandable in <code>app/helpers/review_mapping_helper.rb</code> file. We have have refactored some methods mentioned below for better understanding while maintaining the functionality. The aim is to also make it better understandable for other developers by adding comments wherever necessary.

==Team==
===Mentor===
Devashish Vachhani (dvachha@ncsu.edu)

===Team Members===
Harsh Mauny (hrmauny@ncsu.edu) 
Connor Davidson (cdavids@ncsu.edu) 
Aditya Joshi (ajoshi25@ncsu.edu) 

==Issues to be Addressed==

* '''Issue #1:'''The next several methods generate charts. They are cohesive enough that they should be in their own separate file, either another helper or a mixin.

* '''Issue #2:'''Various method names begin with get. This is not Ruby-like. Change the names to something more appropriate.

* '''Issue #3:'''There are several methods for feedback_response_maps. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.

* '''Issue #4:'''The method sort_reviewer_by_review_volume_desc should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #6:'''The file ends with three small classes being defined. There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

==Plan of work==
We started our work by analysing the <code>review_mapping_helper.rb</code> how it links to rest of the code and discussing the flow of the project in detail with our mentor. Based on our understanding of the code we divided our project requirements in terms of difficulty levels as shown in the [https://docs.google.com/document/d/1AmXVfhH3icn8Pc9oJ3U7okER2NhBDIFFtFxTofmrKcE/edit?usp=sharing document]. then we distributed the tasks among us and started refactoring various methods and creating new files as and when necessary. We also ensured the test cases passing earlier were intact and added new ones if necessary.

=='''Project 4 - DESIGN DOC'''==
Following are the changes we have been assigned to do after completion of project 3

* '''Issue #5:'''Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* '''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow

* '''Issue #8:'''The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* '''Issue #9:'''get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

Apart from these issues we have added some issues of our own based on the feedback of project 3 review which are as follows

* A lot of variables are difficult to understand and hence we plan to refactor the variable for better readability and understanding
* We were not able to increase the code coverage after our changes during project 3. Here in project 4 we want to focus on writing more tests and using the test skeleton.

Below is an UML diagram already uploaded in Expertiza database documentation for reader reference 
[[File:Review mappings imported.png|700px]] 

====Design Patterns====
The Code present in review_mapping_helper can be improved by using design patterns to improve maintainability, readability, and flexibility.
Following are some design patterns that are suggested

* Methods can be extracted into smaller, well maintained with just a single responsibility. This will promote code readability and promote code reuse.

*The methods review_report_data, team_color, obtain_team_color, and others follow a common sequence of steps but allow for variation in some steps. Applying the Template Method Pattern can help define a common structure in a base method while allowing specific steps to be implemented in subclasses or overridden in derived methods.

*Descriptive and short naming conventions are required so the methods and variables are self explanatory.

* Separation of Concerns should be followed for following code.

====Work Plan====

The plan of work on the above issues is as follows 

*'''Issue #5:''' There is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
'''Methods involved:''' <code> list_review_submissions </code> 
'''Problem:''' following are the problems here as stated by the task 
* Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way. 
[[File:E2353-issue-5-orignal-issue.png|700px]] 
'''Solution:''' Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why previous students implemented this method
* Reading the code and the usages of this method to understand how it's being used
* better variable and method names if needed.
 

*'''Issue #7:''' method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow 
'''Methods involved:''' <code> get_data_for_review_report </code> 
'''Problem:''' following are the problems here as stated by the task 
* There are several methods for feedback_response_maps.
* It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing. 
[[File:E2353-issue-7-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* Reading the Expertiza Wiki to understand why this method was originally implemented
* Reading the code and the usages of this method to understand how it's being used
* refactor to make better variable and method names if necessary.
 

*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green. 
[[File:E2353-issue-8-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The colour word has to be replaced by color for consistency
* In order to improve the readability we can reduce the cognitive complexity and branching by using less if-else block and further dividing the method into multiple block.
* More comments can be added to make sure the reader understands what each line does in these methods.
* make sure proper color get assigned based on the review status by making sure all tests pass after refactoring and maybe new test cases can be added
* there are some unwanted array data structure used in order to pass the color code which should be removed to reduce complexity.
* better variable and method names if needed.
 

* '''Issue #9:''' get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.
'''Methods involved:''' <code>get_awarded_review_score</code> 
'''Problem:''' following are the problems here as stated by the task 
* The method computes an overall score based upon scores awarded in individual rounds, but score calculation is being standardized now in response_map.rb and the method needs to be modified to use the new code. 
[[File:E2353-issue-9-orignal-issue.png|700px]] 
'''Solution:'''Below are the solutions proposed 
* The score calculation code in response_map.rb needs to be understood thoroughly and as stated, has to be used in the <code>get_awarded_review_score</code> method
* We plan to contact Nicholas to get further clarity in our understanding of the code before we proceed to use it
* We will add comments as necessary once the standardized code has been utilized in the review mapping helper so that readers can understand what the code fragments are doing
* The current variable names will be improved
 

==Files Modified==

* app/helpers/charts_helper.rb
* app/helpers/review_mapping_helper.rb
* app/helpers/review_mapping_helper.rb
* app/views/reports/_calibration_report.html.erb
* app/views/reports/_feedback_report.html.erb
* app/views/reports/_review_report.html.erb
* app/views/reports/_team_score.html.erb
* app/views/reports/_team_score_score_awarded.html.erb
* spec/features/review_mapping_helper_spec.rb
* spec/helpers/review_mapping_helper_spec.rb

==Project tracking==
please use this template
*'''Issue #[number]:''' text 
'''Methods involved:''' <code>text</code> 
'''Problem:''' text 
'''Solution:'''text, code, images if any 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Name
 

----

* '''Issue #1:''' make a separate file for for chart related elements in <code>review_mapping_helper.rb</code> 
'''Methods involved:''' <code>initialize_chart_elements()</code>, <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code>, <code>calculate_key_chart_information()</code> 
'''Previous Code:''' All these methods were previously a part of the <code>review_mapping_helper.rb</code> when it should have its separate helper file 
'''Problem:''' These methods perform similar tasks of generating graphs and can have there separate module or a mixin and also each of the methods more than 30 lines inside them making them too large 
'''Solution:''' distributed this code among 2 newly created files named <code>review_mapping_charts_helper.rb</code> and <code>data_mapping_helper.rb</code> within helper folder. This way the chart helper methods are separated from review_mapping_helper where they are out of place and difficult for any developer to find. <code>review_mapping_charts_helper.rb</code> contains the original methods and <code>data_mapping_helper.rb</code> manages the data need by these chart methods 

'''review_mapping_charts_helper.rb'''
<pre>
module ReviewMappingChartsHelper
...
def initialize_chart_elements()
def display_volume_metric_chart()
def display_tagging_interval_chart()
def calculate_key_chart_information()
</pre>
 
'''all link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-e2d994d01ebf485444d434603035df8e07eb9db7e625cfc42e7ad83f5b5c333e review_mapping_charts_helper.rb] [https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-ab7a32a37c43b8681f146d9e7e420aee006c8a435e11c45944ac8a13f941c969 data_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #2:''' change methods names starting with "get" to something appropriate 
'''Methods involved:''' <code> get_data_for_review_report()</code>, <code> get_team_color()</code>, <code> get_link_updated_at()</code>, <code> get_team_reviewed_link_name()</code>, <code> get_awarded_review_score()</code>, <code> get_each_review_and_feedback_response_map()</code>, <code> get_certain_review_and_feedback_response_map()</code>, <code> get_css_style_for_calibration_report()</code> 
'''Previous Code:''' previous code used "get" in method names 
'''Problem:''' All these methods used "get_" which is not ruby-like. 
'''Solution:''' change method names to a more ruby-like name that was appropriate for the given context 

'''Contributor:''' Connor Davidson
 
----
*'''Issue #3:''' Several methods for feedback_response_maps, check docs to manage this confusion 
'''Methods involved:''' <code> feedback_response_for_author()</code> 
'''Previous Code:''' previous code had an ambiguous variable name 
'''Problem:''' It wasn't clear what the variable was used for. 
'''Solution:''' refactor the variable name to be more easily understood and add comments to give a clear explanation for why it exists 

'''Contributor:''' Connor Davidson
 
----

*'''Issue #4:''' The method <code>sort_reviewer_by_review_volume_desc</code> should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system. 
'''Methods involved:''' <code>sort_reviewer_by_review_volume_desc</code> 
'''Problem:''' Currently the method sort_reviewer_by_review_volume_desc sorts the reviewers in descending order but only considers one metric which is review volume which makes it very specific and it cannot be used if we want to sort by other metrics 
'''Solution:'''The solution here is make this a generalized method by renaming <code>sort_reviewer_by_review_volume_desc</code> to <code>sort_reviewer_desc</code> and accepting a metric as argument on which we can sort the the reviewers as shown below. Since it was used in <code>review_report</code> partial the implementation was changed there as well 
'''review_mapping_helper.rb''' 
[[File: E2353-Issue-4-1.png|900px]] 
'''app/views/reports/_review_report.html.erb''' 
[[File:E2353issue-4-2.png|900px]] 
'''review_mapping_helper_spec.rb''' 
[[File:E2353-issue4-3.png|900px]] 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-10395a909ed1f2034b4a4c8f57c0e30382d9492e0b16bb6fe6fb91a424f6f034 _review_report.html.erb] 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-1f05f505eaa5edaf0790cdeb6e774cc499685cce4edf12b253ecd358ce710582 review_mapping_helper_spec.rb] 
'''Contributor:''' Harsh Mauny
 
----

*'''Issue #6:''' The Last 3 Classes in review_helper need to be reviewed 
'''Methods involved:''' <code>initialize</code>, <code>reviews_needed</code>, <code>reviews_per_team</code>, <code>reviews_per_student</code> (separate methods for both the sub classes) 
'''Problem:''' The file ends with three small classes being defined. There are no comments at all to explain what is being done 
'''Solution:'''The usage of the three classes was traced back to assignment views and ReviewMappingController. It is evident that these classes have been used to encapsulate the logic for different review strategies in the system. Some comments have been added to explain the functionality of these classes. 
'''review_strategy classes''' 
[[File:E2353-Issue-6.png|900px]] 
'''Further explanation of the methods:''' 
<code>reviews_needed</code> : Calculates the total number of reviews needed for all teams/participants 
<code>reviews_per_team</code> : Calculates the number of reviews each team should receive 
<code>reviews_per_student</code> (for StudentReviewStrategy) : Specifies the number of reviews each student should perform 
<code>reviews_per_student</code> (for TeamReviewStrategy) : Specifies the number of reviews each student should perform based on team assignments 

'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Aditya Joshi
 
----
*'''Issue #8:''' Refactoring the methods providing color coding in review report 
'''Methods involved:''' <code>team_color</code>, <code>obtain_team_color</code>, <code>check_submission_state</code> 
'''Problem:''' following are the problems here as stated by the task 
* American work color has to be used everywhere instead of colour
* The readability and understandability of the code is less
* no proper comments to explain what is going on in the code
* The color coding requirements are met that are, team review is coded as red if review is not completed and blue indicates that the review grade is not assigned or updated. In case the reviewer did not have anything to review in the latest round, and should not be downgraded for not re-reviewing the work, these reviews should be coded green.
Below is the original code
<pre>
def team_color(response_map)
# Storing redundantly computed value in a variable
assignment_created = @assignment.created_at
# Storing redundantly computed value in a variable
assignment_due_dates = DueDate.where(parent_id: response_map.reviewed_object_id)
# Returning color based on conditions
if Response.exists?(map_id: response_map.id)
if !response_map.try(:reviewer).try(:review_grade).nil?
'brown'
elsif response_for_each_round?(response_map)
'blue'
else
obtain_team_color(response_map, assignment_created, assignment_due_dates)
end
else
'red'
end
end

# loops through the number of assignment review rounds and obtains the team color
def obtain_team_color(response_map, assignment_created, assignment_due_dates)
color = []
(1..@assignment.num_review_rounds).each do |round|
check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
end
color[-1]
end

# checks the submission state within each round and assigns team color
def check_submission_state(response_map, assignment_created, assignment_due_dates, round, color)
if submitted_within_round?(round, response_map, assignment_created, assignment_due_dates)
color.push 'purple'
else
link = submitted_hyperlink(round, response_map, assignment_created, assignment_due_dates)
if link.nil? || (link !~ %r{https*:\/\/wiki(.*)}) # can be extended for github links in future
color.push 'green'
else
link_updated_at = get_link_updated_at(link)
color.push link_updated_since_last?(round, assignment_due_dates, link_updated_at) ? 'purple' : 'green'
end
end
end
</pre>
 

'''Solution:''' First we understood the color coding scheme based on the references provided to us and based on which the code was refactored to address the problem. Below are the changes that were made
* The colour word was replaced by color for consistency
* In order to improve the readability the cognitive complexity and branching was reduced by using less if-else block and further dividing the method into multiple block.
* comments were added providing an explanation of what the code does
* making sure proper color get assigned based on the review status
* better variable and method names wherever needed. 
[[File:E2353-issue-8-updated-team-color.png|700px]] 
'''All link related to these changes:''' 
[https://github.com/expertiza/expertiza/pull/2663/files#diff-c41058c9caa3baa134bad689e28b7903d372d6a64388daccbb2e0ced964da568 review_mapping_helper.rb] 
'''Contributor:''' Harsh Mauny
----
==Test Plan==
This is a refactoring project, so it is expected that the original functionality should be retained. Some of the task involved adding comments to previous code, changing variable name etc. which no change was required in test cases. Some methods where method names were changed or they were shifted to other files, for these methods necessary changes were made in respective Rspec files. All the test cases passed which implies that the refactored code did not break the existing functionality.
 
[[File:E2353-Test-Report.png|250px]]

==Github Links==
===Link to Expertiza Repository===
https://github.com/expertiza/expertiza 
===Link to Forked Repository===
https://github.com/adityajoshi1114/expertiza 
===Link to Pull Request===
https://github.com/expertiza/expertiza/pull/2663

File:E2353-issue-8-updated-team-color.png

2023-11-25T05:53:29Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-17T21:01:24Z

Hrmauny: /* Project 4 - DESIGN DOC */

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-17T20:54:32Z

Hrmauny: /* Project 4 - DESIGN DOC */

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-17T20:44:27Z

Hrmauny: /* Project 4 - DESIGN DOC */

File:E2353-issue-9-orignal-issue.png

2023-11-17T20:42:41Z

Hrmauny:

File:E2353-issue-8-orignal-issue.png

2023-11-17T20:41:23Z

Hrmauny:

File:E2353-issue-7-orignal-issue.png

2023-11-17T20:37:06Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-17T20:34:53Z

Hrmauny: /* Project 4 - DESIGN DOC */

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-17T20:34:03Z

Hrmauny: /* Project 4 - DESIGN DOC */

File:E2353-issue-5-orignal-issue.png

2023-11-17T20:33:22Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-14T20:44:19Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-05T01:10:41Z

Hrmauny: /* Plan of work */

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-05T01:04:46Z

Hrmauny: /* Test Plan */

File:E2353-Test-Report.png

2023-11-05T01:03:21Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-05T00:21:26Z

Hrmauny: /* Project tracking */

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-05T00:19:57Z

Hrmauny: /* Project tracking */

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-05T00:18:48Z

Hrmauny: /* Project tracking */

File:E2353-issue4-3.png

2023-11-05T00:18:04Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-04T22:12:41Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-04T22:04:13Z

Hrmauny: /* Project tracking */

File:E2353issue-4-2.png

2023-11-04T22:00:25Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-04T21:56:23Z

Hrmauny: /* Project tracking */

File:E2353-Issue-4-1.png

2023-11-04T21:53:45Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-04T21:25:15Z

Hrmauny: /* Project tracking */

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-04T21:17:18Z

Hrmauny: /* Issues to be Addressed */

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-04T21:13:47Z

Hrmauny: /* Issues to be Addressed */

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-04T21:10:42Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-04T21:01:19Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-04T20:44:41Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-11-03T15:05:11Z

Hrmauny:

==Problem Statement==
Our project concentrates on making things readable and understandable in review_mapping_helper.rb file

==Issues to be Addressed==

* method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow

* The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* Various method names begin with get. This is not Ruby-like. Change the names to something more appropriate.
* get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

* The method sort_reviewer_by_review_volume_desc should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.

* The next several methods generate charts. They are cohesive enough that they should be in their own separate file, either another helper or a mixin.

* Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* There are several methods for feedback_response_maps. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.

* The file ends with three small classes being defined. There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.

==Plan of work==
We started our work by analysing the <code>review_mapping_helper.rb</code> how it links to rest of the code. Based on our understanding of the code we divided our project requirements in terms of difficulty levels as shown in the document [https://docs.google.com/document/d/1AmXVfhH3icn8Pc9oJ3U7okER2NhBDIFFtFxTofmrKcE/edit?usp=sharing]
==Files Modified==

* app/helpers/charts_helper.rb
* app/helpers/review_mapping_helper.rb
* app/helpers/review_mapping_helper.rb
* app/views/reports/_calibration_report.html.erb
* app/views/reports/_feedback_report.html.erb
* app/views/reports/_review_report.html.erb
* app/views/reports/_team_score.html.erb
* app/views/reports/_team_score_score_awarded.html.erb
* spec/features/review_mapping_helper_spec.rb
* spec/helpers/review_mapping_helper_spec.rb

==Project tracking==
* '''Issue #1:''' make a separate file for for chart related elements in review_mapping_helper.rb 
'''Methods involved:''' <code>initialize_chart_elements()</code>, <code>display_volume_metric_chart()</code>, <code>display_tagging_interval_chart()</code>, <code>calculate_key_chart_information()</code> 
'''Previous Code:''' All these methods were previously a part of the <code>review_mapping_helper.rb</code> when it should have its separate helper file 
'''Problem:''' These methods perform similar tasks of generating graphs and can have there separate module or a mixin 
'''Solution:''' Added these methods to an already present helper method <code>charts_helper.rb</code> within helper folder. this way the chart helper method are separted from review_mapping_helper where they are out of place and difficult for any developer to find 

'''charts_helper.rb'''
<pre>
module ChartsHelper
...
def initialize_chart_elements()
def display_volume_metric_chart()
def display_tagging_interval_chart()
def calculate_key_chart_information()
</pre>
 
'''review_mapping_helper.rb'''

<pre>
module ReviewMappingHelper
include ChartsHelper
...
...
...
...</pre>
'''Contributor:''' Harsh Mauny
 

*'''Issue #2:''' change methods names starting with "get" to something appropriate 
'''Methods involved:''' <code> get_data_for_review_report()</code>, <code> get_team_color()</code>, <code> get_link_updated_at()</code>, <code> get_team_reviewed_link_name()</code>, <code> get_awarded_review_score()</code>, <code> get_each_review_and_feedback_response_map()</code>, <code> get_certain_review_and_feedback_response_map()</code>, <code> get_css_style_for_calibration_report()</code> 
'''Previous Code:''' previous code used "get" in method names 
'''Problem:''' All these methods used "get_" which is not ruby-like. 
'''Solution:''' change method names to a more ruby-like name that was appropriate for the given context 

'''Contributor:''' Connor Davidson
 

*'''Issue #7:''' Several methods for feedback_response_maps, check docs to manage this confusion 
'''Methods involved:''' <code> feedback_response_for_author()</code> 
'''Previous Code:''' previous code had an ambiguous variable name 
'''Problem:''' It wasn't clear what the variable was used for. 
'''Solution:''' refactor the variable name to be more easily understood and add comments to give a clear explanation for why it exists 

'''Contributor:''' Connor Davidson

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-10-30T23:21:07Z

Hrmauny: /* Project tracking */

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-10-30T23:16:43Z

Hrmauny:

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-10-30T23:00:00Z

Hrmauny:

==Problem Statement==
Over project concentrates on making things readable and understandable in review_mapping_helper.rb file

==Issues to be Addressed==

* method get_data_for_review_report marshals a lot of data together and passes back a data structure. It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed. It violates the Expert pattern, because the view needs to know how to break apart the structure that is passed to it. Doing the same thing with partials in views/reports would make the code easier to follow

* The next three methods involve team “color”. Color-coding is explained on this page and this page for the E1789 project and this page for E1815. The code for these methods is not at all clear, and should be refactored. And please use the American spelling “color”.

* Various method names begin with get. This is not Ruby-like. Change the names to something more appropriate.
* get_awarded_review_score computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated. Score-calculation code for multiple rounds is being standardized now, in response_map.rb. Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars. Change this method to use the new code.

* The method sort_reviewer_by_review_volume_desc should be generalized so that it can sort by any metric, not just review volume. Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds! Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.

* The next several methods generate charts. They are cohesive enough that they should be in their own separate file, either another helper or a mixin.

* Then there is a method list_review_submissions. It’s not at all clear that this method is needed, though it is used in one view. Look on the Expertiza wiki and see if there is a better way.

* There are several methods for feedback_response_maps. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.

* The file ends with three small classes being defined. There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.

==Files Modified==

* app/helpers/charts_helper.rb
* app/helpers/review_mapping_helper.rb

CSC/ECE 517 Fall 2023

2023-10-30T22:49:21Z

Hrmauny:

* [[CSC/ECE 517 Fall 2023 - E2358. Refactor student_quizzes_controller.rb]]
* [[CSC/ECE 517 Fall 2023 - E2351. Finish mentor management for assignments without topics]]
* [[CSC/ECE 517 Fall 2023 - E2359. Refactor user_controller.rb, user.rb, and its child classes]]
* [[CSC/ECE 517 Fall 2023 - E2367. Reimplement participants_controller.rb, participants.rb and its child classes]]
* [[CSC/ECE 517 Fall 2023 - E2355. Improving Search Facility In Expertiza]]
* [[CSC/ECE 517 Fall 2023 - E2365. Create a user interface for Questionnaire in ReactJS]]
* [[CSC/ECE 517 Fall 2023 - E2364. Create a UI for Course's & Assignment's "Add Participants" page]]
* [[CSC/ECE 517 Fall 2023 - E2361. Create a page to create and update a Questionnaire in ReactJS]]
* [[CSC/ECE 517 Fall 2023 - E2362. Create a page to edit an Assignment's due date in ReactJS]]
* [[CSC/ECE 517 Fall 2023 - E2363. Create a UI for Assignment Edit page "Etc" tab in ReactJS]]
* [[CSC/ECE 517 Fall 2023 - E2375. Reimplement Waitlists]]
* [[CSC/ECE 517 Fall 2023 - E2357. Refactor sign_up_sheet_controller.rb]]
* [[CSC/ECE 517 Fall 2023 - E2366. Reimplement assignment model and assignment controller]]
* [[CSC/ECE 517 Fall 2023 - E2370. Reimplement join team requests controller]]
* [[CSC/ECE 517 Fall 2023 - E2371. Reimplement quiz_questionnaires_controller]]
* [[CSC/ECE 517 Fall 2023 - E2369. Reimplement duties controller.rb and badges controller.rb]]
* [[CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper]]

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-10-30T22:45:59Z

Hrmauny:

==Problem Statement==
Over project concentrates on making things readable and understandable in review_mapping_helper.rb file

CSC/ECE 517 Fall 2023 - E2353. Further refactoring and improvement of review mapping helper

2023-10-30T22:42:52Z

Hrmauny: Created page with "Over project concentrates on making things readable and understandable in review_mapping_helper.rb file"

Over project concentrates on making things readable and understandable in review_mapping_helper.rb file