CSC/ECE 517 Spring 2015/oss E1505 xzl
Expertiza - Refactoring AssignmentParticipant model
Expertiza is a web application developed using Ruby on Rails that serves as a peer-review system. The application allows students to submit and peer-review learning objects (articles, code, web sites, etc)<ref>Expertiza on GitHub</ref><ref>Expertiza Wiki Page</ref>. It is an open source project and it's codebase is maintained in GitHub. We are contributing to Expertiza as a part of our Object-Oriented Design and Development's Open-Source Software (OSS) Project. Our goal in this project is to refactor the StudentQuiz controller. In this Wiki Page, we would be explaining the changes that we have made for the same.
Project Description
The AssignmentParticipant model is subclass of Participant model and is used to maintain the list of students/users participating in a given assignment.
The changes that are needed to be done are described as follows:<ref>GoogleDoc for our project requirements</ref>
- Rename methods prefixed with “get”.
- Move methods to appropriate file helper classes.
- Move methods to appropriate models.
- Remove unused methods
- Use good Ruby style guidelines in the code.
Modifications made to the Existing Code
Rename methods prefixed with “get”
According to Ruby naming conventions, the name of getter and setter methods need not to be verbs and they should not be prefixed with "get".
The following methods were renamed.
get_reviewers => reviewers get_scores => scores get_members => members get_team_hyperlinks => team_hyper_links get_hyperlinks_array => hyperlinks_array get_course_string => course_string get_feedback => feedback get_reviews => reviews self.get_export_fields => self.export_fields get_path => path get_current_stage => current_stage def get_stage_deadline => def stage_deadline
Move methods to appropriate file helper classes
Methods get_submitted_files, get_files should not be in this class. They deal with files and should be moved to appropriate file helper classes.
Move methods to appropriate models
reviewed_by? , quiz_taken_by? do not belong to AssignmentParticipant model. Move them to appropriate models. File:DeleteFile.png
Remove unused methods
In cases where a method only returns a boolean value by evaluating an expression (using the if..else construct), the if..else statement is redundant and can be eliminated. We have a method graded? that does this thing. Thus, we eliminated the if..else construct in the graded? method as it was redundant.<ref>About Boolean Zen</ref>
Before Refactoring
Method : graded?
def graded?(response, question)
if Score.where(question_id: question.id, response_id: response.id).first
return true
else
return false
end
end
After Refactoring
Method : graded?
def graded?(response, question) return (Score.where(question_id: question.id, response_id: response.id).first) end
Use good Ruby style guidelines in the code
At many places in the code we found that the Ruby Style Guidelines were not followed. We have refactored the code so that it uses the good code style guidelines. The following are some of the refactorings the we have done:-
Used .eql? instead of "=="
Before Refactoring
Method : finished_quiz
if score.score == -1
After Refactoring
if score.score.eql? -1
Eliminated the "== true/false" check
Before Refactoring
Method : finished_quiz
if essay_not_graded == true
After Refactoring
if essay_not_graded
Use `&&` and `||` rather than `and` and `or` to keep boolean precedence
Before Refactoring
Method: record_response
elsif correct_answer and params["#{question.id}"] == correct_answer.txt
After Refactoring
Method: calculate_score
elsif correct_answer && params["#{question.id}"]== correct_answer.txt
Use key: ‘value’, not :key => ‘value’
Before Refactoring
Method: record_response
new_score = Score.new :comments => choice, :question_id => question.id, :response_id => @response.id
After Refactoring
Method: calculate_score
new_score = Score.new comments: choice, question_id: question.id, response_id: response.id
Replace find_by_... with a where command
Before Refactoring
Method: record_response
if (QuestionType.find_by_question_id question.id).q_type == 'MCC'
After Refactoring
Method: calculate_score
ques_type = (QuestionType.where( question_id: question.id)).q_type if ques_type.eql? 'MCC'
Used .nil? instead of "== nil"
Before Refactoring
Method: record_response
if params["#{question.id}"] == nil
After Refactoring
Method: calculate_score
if params["#{question.id}"].nil?
Used a Boolean variable when that is sufficient
Before Refactoring
Method: record_response
valid = 1 if valid == 0
After Refactoring
Method: calculate_score
valid = false if valid
Use good conditional statements
Using unless is a good practice. But it is not a good practice to use unless and ! within the condition. Instead we could use an if condition itself.
Before Refactoring
Method: record_response
unless new_score.comments != "" && new_score.comments valid = false
After Refactoring
Method: calculate_score
if new_score.comments.empty? || new_score.comments.nil? valid = false
Use of Routing Helpers
Routing helpers are a simpler alternative to the otherwise complex hard coded URLs which reduce the readability of the code. Routing helpers allow us to declare possible common routes for a given controller. Routing helpers have been implemented since they maintain consistency even if changes are made to the routing paths.
Before Refactoring:
config.rb does not contain a student_quizzes resource
review_mapping_controller.rb
redirect_to :controller => 'student_quizzes', :action => 'index', :id => params[:participant_id]
After Refactoring
config.rb
resources :student_quizzes, :only => [:index]
review_mapping_controller.rb
redirect_to student_quizzes_path(:id => params[:participant_id])
Replace controller method with a model method
Rails 4 conventions dictate the use of a fat model and skinny controller. It is better to place the search function in the model rather than placing it in the controller. The search code belonged to the quiz_response_map model, since it queries that particular table in the database. The code was extracted into the get_mappings_for_reviewer method and placed in the file quiz_response_map.rb. This method was then called from the StudentQuizzes controller. <ref name = "stackoverflow">Fat Models and Skinny Controllers.</ref>
Before Refactoring
student_quizzes_controller.rb
Method : index
@quiz_mappings = QuizResponseMap.where(reviewer_id: participant.id)
After Refactoring
student_quizzes_controller.rb
Method : index
@quiz_mappings = QuizResponseMap.get_mappings_for_reviewer(participant.id)
quiz_response_map.rb
def self.get_mappings_for_reviewer(participant_id) return QuizResponseMap.where(reviewer_id: participant_id) end
Changes made in method logic
We have made certain changes in the logic of the methods calculate_score and record_response (previously the code of both these methods was only in record_response) primarily to improve the existing logic and eliminate redundant code. These changes are described as follows:
- The score variable was already being set to 0 on the loop entry, therefore it was redundant to reset score to zero again. Thus, we eliminated this line of code inside the if statement.
Before Refactoring
questions.each do |question|
score = 0
if (QuestionType.find_by_question_id question.id).q_type == 'MCC'
score = 0
After Refactoring
questions.each do |question|
score = 0
if ques_type.eql? 'MCC'
# Eliminated score = 0 over here
- The variable correct_answer stored multiple values as the where condition to which it was assigned was returning multiple values. Therefore it seemed more intuitive to rename correct_answer to correct_answers so that it is apparent that it contains multiple values.
Before Refactoring
correct_answer = QuizQuestionChoice.where(question_id: question.id, iscorrect: 1)
After Refactoring
correct_answers = QuizQuestionChoice.where(question_id: question.id, iscorrect: true)
- The below piece of code found out the question type twice in the same loop. Therefore we extracted it and assigned it to a local variable so that the query is executed only once.
Before Refactoring
# First Query if (QuestionType.find_by_question_id question.id).q_type == 'MCC' # Repetition of the query in the same loop if (QuestionType.find_by_question_id question.id).q_type == 'Essay'
After Refactoring
# Querying only once and assigning it to a local variable ques_type ques_type = (QuestionType.where( question_id: question.id)).q_type # Usage 1 of ques_type if ques_type.eql? 'MCC' # Usage 2 of ques_type if ques_type.eql? 'Essay'
- The new_scores and scores array stored almost the similar values. The scores array contained a copy of the value that the new_scores array contained. Therefore we eliminated the new_scores array and are performing all the operations only on the scores array.
Before Refactoring
# Part 1 new_score = Score.new :comments => choice, :question_id => question.id, :response_id => @response.id unless new_score.valid? valid = 1 end new_scores.push(new_score) # Part 2 new_scores.each do |score_update| score_update.score = score scores.push(score_update) end
After Refactoring
# Part 1 new_score = Score.new comments: choice, question_id: question.id, response_id: response.id unless new_score.valid? valid = false end scores.push(new_score) # Part 2 scores.each do |score_update| score_update.score = score end
- The logic to compute the final score for a multiple-choice, multiple-correct type of question seemed to be incorrect and therefore we fixed it.
Before Refactoring
# Part 1 of the Scoring Logic
questions.each do |question|
score = 0
if (QuestionType.find_by_question_id question.id).q_type == 'MCC'
score = 0
if params["#{question.id}"] == nil
valid = 1
else
correct_answer = QuizQuestionChoice.where(question_id: question.id, iscorrect: 1)
params["#{question.id}"].each do |choice|
correct_answer.each do |correct|
if choice == correct.txt
score += 1
end
# Part 2 of the scoring logic which seems to award full points even if you marked some extra options apart from marking all correct answers
unless score == correct_answer.count
score = 0
else
score = 1
end
After Refactoring
# Part 1 of the Scoring Logic
questions.each do |question|
score = 0
correct_answers = QuizQuestionChoice.where(question_id: question.id, iscorrect: true)
ques_type = (QuestionType.where( question_id: question.id)).q_type
if ques_type.eql? 'MCC'
if params["#{question.id}"].nil?
valid = false
else
params["#{question.id}"].each do |choice|
correct_answers.each do |correct|
if choice.eql? correct.txt
score += 1
end
# Part 2 of the scoring logic - We have also compared the number of options the user selected to the total number of correct answers
if score.eql? correct_answers.count && score == params["#{question.id}"].count
score = 1
else
score = 0
end
- The record_response function was performing two distinct operations : One operation was saving the response to the database and the other was to calculate the score for the questions. We created a new function calculate_score that would calculate the score for the questions and record_response now only performs the task of saving responses to the database. By doing this, we followed the Single Responsibility O-O Design Principle for methods mentioned by Mr. Robert C. Martin which states that "Every software module should have only one reason to change"<ref>O-O Design Principles</ref>
Before Refactoring
def record_response
@map = ResponseMap.find(params[:map_id])
@response = Response.new()
@response.map_id = params[:map_id]
@response.created_at = DateTime.current
@response.updated_at = DateTime.current
@response.save
@questionnaire = Questionnaire.find(@map.reviewed_object_id)
scores = Array.new
new_scores = Array.new
valid = 0
questions = Question.where(questionnaire_id: @questionnaire.id)
questions.each do |question|
score = 0
if (QuestionType.find_by_question_id question.id).q_type == 'MCC'
score = 0
if params["#{question.id}"] == nil
valid = 1
else
correct_answer = QuizQuestionChoice.where(question_id: question.id, iscorrect: 1)
params["#{question.id}"].each do |choice|
correct_answer.each do |correct|
if choice == correct.txt
score += 1
end
end
new_score = Score.new :comments => choice, :question_id => question.id, :response_id => @response.id
unless new_score.valid?
valid = 1
end
new_scores.push(new_score)
end
unless score == correct_answer.count
score = 0
else
score = 1
end
new_scores.each do |score_update|
score_update.score = score
scores.push(score_update)
end
end
else
score = 0
correct_answer = QuizQuestionChoice.where(question_id: question.id, iscorrect: 1).first
if (QuestionType.find_by_question_id question.id).q_type == 'Essay'
score = -1
elsif correct_answer and params["#{question.id}"] == correct_answer.txt
score = 1
end
new_score = Score.new :comments => params["#{question.id}"], :question_id => question.id, :response_id => @response.id, :score => score
unless new_score.comments != "" && new_score.comments
valid = 1
end
scores.push(new_score)
end
end
if valid == 0
scores.each do |score|
score.save
end
redirect_to :controller => 'student_quizzes', :action => 'finished_quiz', :map_id => @map.id
else
flash[:error] = "Please answer every question."
redirect_to :action => :take_quiz, :assignment_id => params[:assignment_id], :questionnaire_id => @questionnaire.id
end
end
After Refactoring
# New record_response method
def record_response
map = ResponseMap.find(params[:map_id])
response = Response.new
response.map_id = params[:map_id]
response.created_at = DateTime.current
response.updated_at = DateTime.current
response.save
calculate_score map,response
end
# New calculate_score
def calculate_score map, response
questionnaire = Questionnaire.find(map.reviewed_object_id)
scores = Array.new
valid = true
questions = Question.where(questionnaire_id: questionnaire.id)
questions.each do |question|
score = 0
correct_answers = QuizQuestionChoice.where(question_id: question.id, iscorrect: true)
ques_type = (QuestionType.where( question_id: question.id)).q_type
if ques_type.eql? 'MCC'
if params["#{question.id}"].nil?
valid = false
else
params["#{question.id}"].each do |choice|
correct_answers.each do |correct|
if choice.eql? correct.txt
score += 1
end
end
new_score = Score.new comments: choice, question_id: question.id, response_id: response.id
unless new_score.valid?
valid = false
end
scores.push(new_score)
end
if score.eql? correct_answers.count && score == params["#{question.id}"].count
score = 1
else
score = 0
end
scores.each do |score_update|
score_update.score = score
end
end
else
correct_answer = correct_answers.first
if ques_type.eql? 'Essay'
score = -1
elsif correct_answer && params["#{question.id}"]== correct_answer.txt
score = 1
end
new_score = Score.new :comments => params["#{question.id}"], :question_id => question.id, :response_id => response.id, :score => score
if new_score.comments.empty? || new_score.comments.nil?
valid = false
end
scores.push(new_score)
end
end
if valid
scores.each do |score|
score.save
end
redirect_to :controller => 'student_quizzes', :action => 'finished_quiz', :map_id => map.id
else
flash[:error] = "Please answer every question."
redirect_to :action => :take_quiz, :assignment_id => params[:assignment_id], :questionnaire_id => questionnaire.id
end
end
References
<references/>