CSC517 (Spring 2020) - E2010. Refactor criterion.rb
E2010 Refactor criterion.rb
This page provides a description of the Expertiza based OSS project.
About Expertiza
Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.
Problem Statement
The criterion.rb model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So, this file consists of a lot of HTML code that is rendered to the final questionnaire view.
The current version of this controller has four methods, but two methods are very long and would require refactoring. The primary problem with these functions is that it consists of many statements of string concatenation. The HTML code which is rendered to the final view is made up of concatenations.
Another issue with the code is that the branch conditions size for the complete and the view_completed_question method is very high. Also, this controller has very few comments. They are specifically needed to differentiate between the purpose of numerous nested branches from each other.
About criterion.rb
The criterion.rb model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So this file consists of a lot of HTML code that is rendered to the final questionnaire view.
For this project, we have to deal with the following two methods:
- The complete method, which is 104 lines long. this method returns the display for the students when they are filling the questionnaire. It includes the advice given for different questions, dropdown options to rate a project based on the question, a textarea to enter comments and so on.
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end
html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
# show advice for each criterion question
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0
question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end
if !question_advices.empty? and advice_total_length > 0
html += '<a id="showAdivce_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a>'
html += '<script>'
html += 'function showAdvice(i){'
html += 'var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){'
html += 'element.innerHTML="Show advice";'
html += '}else{'
html += 'element.innerHTML="Hide advice";}'
html += 'toggleAdvice(i);}'
html += 'function toggleAdvice(i) {'
html += 'var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {'
html += 'elem.style.display = "";'
html += '} else {'
html += 'elem.style.display = "none";}}'
html += '</script>'
html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/>'
html += '<script>'
html += 'function changeScore(i, j) {'
html += 'var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}'
html += '</script>'
end
html += '</div>'
end
if dropdown_or_scale == 'dropdown'
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?
html += '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>'
html += "<option value = ''>--</option>"
questionnaire_min.upto(questionnaire_max).each do |j|
html += if !answer.nil? and j == answer.answer
'<option value=' + j.to_s + ' selected="selected">'
else
'<option value=' + j.to_s + '>'
end
html += j.to_s
if j == questionnaire_min
html += "-" + self.min_label if self.min_label.present?
elsif j == questionnaire_max
html += "-" + self.max_label if self.max_label.present?
end
html += "</option>"
end
html += "</select></div><br><br>"
html += '<textarea' + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea></td>'
elsif dropdown_or_scale == 'scale'
html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil?
html += '>'
html += '<table>'
html += '<tr><td width="10%"></td>'
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end
html += '<td width="10%"></td></tr><tr>'
html += if !self.min_label.nil?
'<td width="10%">' + self.min_label + '</td>'
else
'<td width="10%"></td>'
end
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
html += '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {'
html += 'var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script>'
html += if !self.max_label.nil?
'<td width="10%">' + self.max_label + '</td>'
else
'<td width="10%"></td>'
end
html += '<td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea>'
end
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
- The view_completed_question method, which is 47 lies long. Thismethod is responsible to return the display if a student is viewingan already filled-out questionnaire.
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"
score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = if score != "-"
answer.answer * 1.0 / questionnaire_max
else
0
end
score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end
html += '<table cellpadding="5">'
html += '<tr>'
html += '<td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score
html += '</div>'
html += '</td>'
if answer && !answer.comments.nil?
html += '<td style="padding-left:10px">'
html += '<br>' + answer.comments.html_safe
html += '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
end
Proposed Solution
Our goal is to refactor the complete and view_completed_question method mainly by reducing the number of lines code for each function and also by introducing new methods to make the code more modular. We will also try to reduce the branch condition size wherever possible and hence reduce the cyclomatic complexity for these two functions. We also plan to introduce comments wherever needed to make the code more understandable. The main aim is to reduce the number of lines and make the code more compact without affecting the readability of the code.
Implementation
We tried to tackle the issues mentioned in the problem statement as described below:
The complete method
All the code in the model is just HTML. We thought of removing the code from here and putting it in a .erb file but it didn’t make much sense as the HTML code was heavily branched by the if-else conditions and it would just have been taking it out from this function and putting it in another file. We instead decided to refactor it in the file. We mainly combined the short HTML strings into longer ones but not too long so that the Code Climate cannot throw a “line too long” error. Another thing we did is reduce the number of lines used by condition statements by using the shorthand which beautifully consolidates three or more lines of code (read: if-else statements) into one readable line of code. (We also created another method for the repeated HTML code, so we just have to call the method rather than repeating the code. It increases code re-usability and reduces the number of lines of code.)
List of changes
The modified complete method
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end
html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
# show advice for each criterion question
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0
question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end
if !question_advices.empty? and advice_total_length > 0
html += '<a id="showAdvice_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a><script>'
html += 'function showAdvice(i){var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){element.innerHTML="Show advice";} else{element.innerHTML="Hide advice";}toggleAdvice(i);}'
html += 'function toggleAdvice(i) {var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {elem.style.display = "";} else {elem.style.display = "none";}}</script>'
html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/><script>'
html += 'function changeScore(i, j) {var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}</script>'
end
html += '</div>'
end
if dropdown_or_scale == 'dropdown'
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?
html += '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>' + "<option value = ''>--</option>" + '<option value='
questionnaire_min.upto(questionnaire_max).each do |j|
html += j.to_s + 'selected="selected"' if !answer.nil? and j == answer.answer + '>'
html += j.to_s + "-"
html += self.min_label if self.min_label.present? and j == questionnaire_min + "</option>"
html += self.max_label if self.max_label.present? and j == questionnaire_max + "</option>"
end
html += '</select></div><br><br><textarea' + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil? + '</textarea></td>'
elsif dropdown_or_scale == 'scale'
html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil? + '><table><tr><td width="10%"></td>'
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end
html += '<td width="10%"></td></tr><tr><td width="10%">' + self.min_label if !self.min_label.nil? + '</td>'
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j) + '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script><td width="10%">'
html += self.max_label if !self.max_label.nil? + '</td><td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil? + '</textarea>'
end
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
The view_completed_question" method
The main task here was to reduce the nested if-else branching and reduce the number of arguments for this function. This was a very small method with its major dependent functions present in response.rb file. To refactor this method, we would have had to refactor response.rb and in turn, the other question subclasses which would make it very complex and break the code. According to us, refactoring this method along with the response.rb file would make more sense. We still made some changes in the file like concatenating small HTML string and using shorthand for if-else condition statements.
The modified view_completed_question method is as below:
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"
score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = score != "-" ? answer.answer * 1.0 / questionnaire_max : 0
score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end
html += '<table cellpadding="5"><tr><td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score + '</div></td>'
if answer && !answer.comments.nil?
html += '<td style="padding-left:10px"><br>' + answer.comments.html_safe + '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end