CSC/ECE 517 Fall 2014/OSS E1466 gjf
E1466: Refactoring GradesController
This wiki deals with our implementation of a controller in expertiza: grade_controller for the Expertiza Project using Ruby on Rails.
Introduction
Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities.
Project Description
Classes involved: grades_controller.rb
What it does:
This class lists the grades of all the participants for an assignments and also their reviews. Instructor can edit scores, calculate penalties and send emails for conflicts.
What needs to be done:
- Modify calculate_all_penalties method which is too complex and long.
- Put the get_body_text method in a mailer rather than in the grades_controller.
- Refactor conflict_nofication method to conflict_email and make it delegated to the mailer.
- Refactor view_my_scores method to grades_show and delete the unnecessary variables.
- Try not to query for reviews and meta reviews in grades_show method.
What we have done:
- Modify calculate_all_penalties method.
- Refactor the conflict_notification method to conflict_email method.
- Simplify sending email function and make it delegated to the mailer.
- Send a conflict email to the reviewer automatically when instructor click the button "email reviewer".
- Remove get_body_text and send_grading_email method from grades_controller.
- Refactor conflict_nofication method to conflict_email and make it delegated to the mailer.
- Use Design Pattern Delegation in ConflictMailer.
- Refactor view_my_scores method to grades_show method.
- Search for the key reasons which lead to huge waiting time for getting score.
- Refactor get_assessments_for method in response_map.rb and lead to more than 90% off the current waiting time.
- Eliminate the search for reviews and meta reviews during the grades_show method.
- Delete unnecessary instance Variables.
Set Up Environment
User Guide
Current System (After Refactoring Grades_controller):
Instructor: user6 Password: test
Student: user1600 and user1601 Password: test
Original System (Before Refactoring Grades_controller):
Instructor: user6 Password: test
Student: user1600 and user1601 Password: test
All our test result based on the following test cases on expertiza, please follow these step to get it.
Instructor: (Searching "Program 2" using "Ctrl + F" will be convinient for you.)
Steps: Login -> Assignments->Program 2 style ->view scores.
Student:
Steps: Login -> Assignments->Program 2 style ->Your scores.
Set Up Environment Locally
Get Expertiza from Github
git clone https://github.com/maxlpy/expertiza.git
Use rvm to install ruby-2.1.0
curl -L https://get.rvm.io | bash -s stable
source ~/.rvm/scripts/rvm
echo "source ~/.rvm/scripts/rvm" >> ~/.bashrc
rvm install 2.1.0
rvm use 2.1.0 --default
ruby -v
Install Bundled Gems
Set JAVA_HOME for the rjb gem: Your path may be different. You can generally find out the path by looking at the symbolic link at /etc/alternatives/java
ls -la /etc/alternatives/java
This outputs something like '/usr/lib/jvm/java-6-openjdk-amd64/jre/bin/java'. Only part of this path may need to be set to JAVA_HOME. In this instance, it is '/usr/lib/jvm/java-6-openjdk-amd64'.
export JAVA_HOME=/usr/lib/jvm/java-6-openjdk-amd64
bundle install
Set Up the Database
Enable and Start the MySql Daemon
sudo service mysqld enable
sudo service mysqld start
Set the MySql Root Password
mysqladmin -u root password
Log in to MySql
mysql -uroot -p
Create the Databases
rake db:create:all
Build the Expertiza Database
rake db:migrate
Start Expertiza service
rails server
Code Modification
calculate_all_penalties Method
Summary
We have modified the calculate_penalties method and implemented all the functions it originally has. After our modification the method shrinks from 45 lines to 32 lines of Ruby code. Originally, the method includes a lot of unnecessary if and unless statements which makes the method uneasy to read and seems to have a difficult logic to calculate the penalties.
According to the codeclimate which calculate the complexity of the method, originally, it has a complexity of 85, however, after we modified it, the complexity goes down to 65. Please check at original version,modified version
We have done the following to the original methods:
- Used for loop with selective choice to create penalty attributes and put it into the CalculatedPenalty Table, instead of duplicating the almost same code.
- Used Object-Oriented Design patterns of Ruby: let the array respond to the method min which returns the minimum number in the array and get the logical penalty other than using if statement.(max_penalty if total_penalty is larger than max_penalty, total_penalty if total_penalty is smaller than max_penalty)
- Eliminated the unnecessary if statements to make the code clear to read.
Implementation
- Modify if statements:
Before Refactoring:
if(penalties[:submission].nil?)
penalties[:submission]=0
end
if(penalties[:review].nil?)
penalties[:review]=0
end
if(penalties[:meta_review].nil?)
penalties[:meta_review]=0
end
After Refactoring:
penalties[:submission] = 0 if penalties[:submission].nil? penalties[:review] = 0 if penalties[:review].nil? penalties[:meta_review] = 0 if penalties[:meta_review].nil?
- Using Objected-oriented Pattern
Before Refactoring:
if(@total_penalty > l_policy.max_penalty)
@total_penalty = l_policy.max_penalty
end
After Refactoring:
@total_penality=[l_policy.max_penalty,@total_penality].min
- Using loops rather than duplicate same code
Before Refactoring:
penalty_attr1 = {:deadline_type_id => 1,:participant_id => @participant.id, :penalty_points => penalties[:submission]}
CalculatedPenalty.create(penalty_attr1)
penalty_attr2 = {:deadline_type_id => 2,:participant_id => @participant.id, :penalty_points => penalties[:review]}
CalculatedPenalty.create(penalty_attr2)
penalty_attr3 = {:deadline_type_id => 5,:participant_id => @participant.id, :penalty_points => penalties[:meta_review]}
CalculatedPenalty.create(penalty_attr3)"
@all_penalties[participant.id] = Hash.new
@all_penalties[participant.id][:submission] = penalties[:submission]
@all_penalties[participant.id][:review] = penalties[:review]
@all_penalties[participant.id][:meta_review] = penalties[:meta_review]
@all_penalties[participant.id][:total_penalty] = @total_penalty"
After Refactoring:
deadline_type=[1,2,5]
penalty_type=[:submission,:review,:meta_review]
if calculate_for_participants
for i in 0..2
penalty_attr={:deadline_type_id=>deadline_type[i],:participant_id => @participant.id, :penalty_points => penalties[penalty_type[i]]}
CalculatedPenalty.create(penalty_attr)
end
end
@all_penalties[participant.id] = {}
for i in 0..2
@all_penalties[participant.id][:penalty_type[i]] = penalties[:penalty_type[i]]
end
@all_penalties[participant.id][:total_penalty] = @total_penalty"
conflict_notification Method
Summary
We refactored the conflict_notification method to conflict_email method. When instructor click the button "email reviewer" whom he thought give a unfair review, the system should be able to send an email to the reviewer automatically.The get_body_text and send_grading_email were removed from the controller. The sending email function is well simplified and delegated to a conflict_mailer.
Design Pattern
Delegation: The conflict_email should be able to send email. The class ConflictMailer works as its delegate .The conflict_email calls the send_conflict_email function to take the job of sending an email.
UML: Draw UML graph for Grades_controller
Implementation
The old conflict_notification action queries an email address list of a certain kind of reviews. In its view conflict_notifiction.html.erb, A email form contains email list for instructor to choose from and email content.
Before Refactorring, in conflict_notification: First,Get the instructor email address as sender
if session[:user].role_id !=6
@instructor = session[:user]
else
@instructor = Ta.get_my_instructor(session[:user].id)
end
@participant = AssignmentParticipant.find(params[:id])
@assignment = Assignment.find(@participant.parent_id)
Then,Based on submission of review, the action query all the reviewers' email address
@questions = Hash.new
questionnaires = @assignment.questionnaires
questionnaires.each {
|questionnaire|
@questions[questionnaire.symbol] = questionnaire.questions
}
The email list is for instructor to choose from. The process_response method queries the email address.
@reviewers_email_hash = Hash.new
@caction = "view"
@submission = params[:submission]
if @submission == "review"
@caction = "view_review"
@symbol = "review"
process_response("Review", "Reviewer", @participant.reviews, "ReviewQuestionnaire")
elsif @submission == "review_of_review"
@symbol = "metareview"
process_response("Metareview", "Metareviewer", @participant.metareviews, "MetareviewQuestionnaire")
elsif @submission == "review_feedback"
@symbol = "feedback"
process_response("Feedback", "Author", @participant.feedback, "AuthorFeedbackQuestionnaire")
elsif @submission == "teammate_review"
@symbol = "teammate"
process_response("Teammate Review", "Reviewer", @participant.teammate_reviews, "TeammateReviewQuestionnaire")
end
The get_body_text method construct email content which should be removed from controller.
@body = get_body_text(params[:submission])
The conflict_notification action requiring another two method "process_response" and "get_body_text" method to get the recipient email list and email content which contributes to extra complexity. The instructor only have to click the button "send email" in conflict_notifiction.html.erb and turn to send_conflict_email action, then he/she can send the email. If the instructor want to send another email, the process will repeat again,which will be terrible when reloading the "view" scores page.
After Refactoring:The querying reviews actually have done in our show_reviews action, and we can get the individual reviewers' email address.In the reviewer table,we add extra button "email reviewer".Now the instructor could send to reviewer email directly.
In _review_table.html.erb,we add the link to send email. The review_id is passed to the conflict_email action taken as the recipient.
<%= link_to 'email reviewer', :action => 'conflict_email',:remote=> true,:reviewer_id=>
review.map.reviewer.user,:participant_id=>participant,:score=>sprintf("%.2f",score) %>
In our new conflict_email action:
We still need the instructor as the sender
if session[:user].role_id !=6
@instructor = session[:user]
else
@instructor = Ta.get_my_instructor(session[:user].id)
end
The reviewer_id is passed from the link,we can query his/her email address to send the email
recipient=User.find(params[:reviewer_id])
The responsibility of sending email has been delegated to a new mailer conflict_mailer. And the email content was constructed in the mailer.
participant = AssignmentParticipant.find(params[:participant_id])
assignment=Assignment.find(participant.parent_id)
score=params[:score]
ConflictMailer.send_conflict_email(instructor,recipient,assignment,score).deliver
Take a look at new conflict_mailer, it define a send_conflict_email method. the email was sent from sender's email and to reviewer's email.Some instance variables were used in view. In conflict_mailer.erb
def send_conflict_email(sender,recipient,assignment,score)
@assignment=assignment
@recipient=recipient
@score=score
if recipient.role_id==1
@role = "reviewer"
@item = "submission"
else
@role = "metareviewer"
@item = "review"
end
mail subject: "Confliction Email",from: sender.email,to: reviewer.email
end
In its view send_conflict_email.txt.haml, we edit the email content.By this way, we deleted the get_body_text and well followed the rule of MVC design
%h1="Hi ," +@recipient.name %p="You submitted a score of "+@score+" for assignment "+@assignment.name+" that varied greatly from another "+@role+"'s score for the same "+@item+"." %p="The Expertiza system has brought this to my attention."
The link "email reviewer" request a ajax,so the current view page will not refresh when the action done
respond_to do |format|
format.js
end
If an email was successfully sent,The instructor should get the alert window by ajax.
view_my_scores method
Summary
We refactored view_my_scores method to grades_show, deleted many unnecessary instance variables along with several review and meta reviews methods inside it. Therefore, the system will only search for scores in the database rather than search for scores and reviews, which wastes a lot of time during the grade show process. After refactoring, the complexity on this decreased from 53 to 35. We also optimize get_scores method to improve the efficiency of showing scores by students and instructors, which cost us most of the time during the project. After refactoring this method, the time cost of views in grade controller decreased by more than 90%.
We have done the following to the original methods:
- Refactored view_my_scores method to grades_show method.
- Searched for the key reasons which lead to huge waiting time for getting score.
- Refactored get_assessments_for method in response_map.rb and lead to more than 90% off the current waiting time.
- Eliminated the search for reviews during the grades_show method.
- Deleted unnecessary instance Variables.
Implementation
- Deleted the unnecessary instance variables
Deleted Code:
@average_score_results = Array.new
@average_score_results = ScoreCache.get_class_scores(@participant.id)
@statistics = Array.new
@average_score_results.each { |x|
@statistics << x
}
- Deleted query for reviews and meta_reviews
Deleted Code:
@average_reviews = ScoreCache.get_reviews_average(@participant.id) @average_metareviews = ScoreCache.get_metareviews_average(@participant.id) @my_reviews = ScoreCache.my_reviews(@participant.id) @my_metareviews = ScoreCache.my_metareviews(@participant.id)
- Modify get_assessments_for method in response_map.rb
After doing this, the time cost of view function decreased by more than 90% Before Refactoring:
# the original method to find all response
@all_resp=Response.all
for element in @all_resp
if (element.map_id == map.map_id)
@array_sort << element
@test << map
end
end
After Refactoring:
@all_resp=Response.find_by_map_id(map.map_id) @array_sort << @all_resp @test << map
- Add a new method into grades_controller.rb
Add Method:
#show_reviews method gets reviews, meta reviews and author feedback information from server.
def show_reviews
@partial='grades/'+params[:path]
@prefix=params[:prefix]
@score=Hash.new
@score[:assessments]=Array.new
params[:score][:assessments].each do |assessment|
@score[:assessments]<<Response.find(assessment)
end
@score[:scores]=params[:score][:scores]
@participant=AssignmentParticipant.find(params[:participant])
@assignment = @participant.assignment
@questions = {}
questionnaires = @assignment.questionnaires
questionnaires.each do |questionnaire|
@questions[questionnaire.symbol] = questionnaire.questions
end
end
- Change view_my_score.html.erb into show_reviews.html.erb in views.
- Add in views(show_reviews.html.erb)
<%= render :partial=>@partial, :locals => {:prefix => @prefix, :participant => @participant, :rscore => @score} %>
Test Result
- The following test results based on the test case on Expertiza(Assignments: Program 2 style)
| Name | Before Refactoring | After Refactoring | Reduced By |
|---|---|---|---|
| View all team's score(instructor) | 484988ms | 8642ms | 98.21% |
| View own score(student) | 8941ms | 651ms | 92.71% |
- Original Time for Instructor to View all scores
- Time for Instructor to View all scores after Refactoring
- Original Time for Student to View all scores
- Time for Student to View all scores after Refactoring
