CSC/ECE 517 Spring 2016/Refactor and write unit tests for question type.rb: Difference between revisions
No edit summary |
|||
(14 intermediate revisions by 2 users not shown) | |||
Line 43: | Line 43: | ||
* Cyclomatic complexity for complete is too high: Line 35 - 158 [28/6] | * Cyclomatic complexity for complete is too high: Line 35 - 158 [28/6] | ||
Wrap the code into different instance methods to reduce cyclomatic complexity | Wrap the code into different instance methods to reduce cyclomatic complexity | ||
< | <pre> | ||
def complete(count, answer=nil, questionnaire_min, questionnaire_max, dropdown_or_scale) | def complete(count, answer=nil, questionnaire_min, questionnaire_max, dropdown_or_scale) | ||
if self.size.nil? | if self.size.nil? | ||
Line 52: | Line 52: | ||
rows = self.size.split(',')[1] | rows = self.size.split(',')[1] | ||
end | end | ||
html = '<li><div><label for="responses_' +count.to_s+ '">' +self.txt+ | html = '<li><div><label for="responses_' +count.to_s+ '">' +self.txt+ "</label></div>" | ||
#show advice for each criterion question | #show advice for each criterion question | ||
question_advices = QuestionAdvice.where(question_id: self.id).sort_by { |advice| advice.id } | question_advices = QuestionAdvice.where(question_id: self.id).sort_by { |advice| advice.id } | ||
Line 83: | Line 83: | ||
html.html_safe | html.html_safe | ||
end | end | ||
</ | </pre> | ||
* Identical code found in 1 other location: Line 20 - 32 (mass = 89) | * Identical code found in 1 other location: Line 20 - 32 (mass = 89) | ||
Also found in app/models/scale.rb:17…28. The following method is removed from subclass: criterion and added to their superclass: scaled_question | |||
<pre> | |||
def view_question_text | |||
html = '<TR><TD align="left"> '+self.txt+' </TD>' | |||
html += '<TD align="left">'+self.type+'</TD>' | |||
html += '<td align="center">'+self.weight.to_s+'</TD>' | |||
questionnaire = self.questionnaire | |||
if !self.max_label.nil? && !self.min_label.nil? | |||
html += '<TD align="center"> ('+self.min_label+') '+questionnaire.min_question_score.to_s+' to '+ questionnaire.max_question_score.to_s + ' ('+self.max_label+')</TD>' | |||
else | |||
html += '<TD align="center">'+questionnaire.min_question_score.to_s+' to '+ questionnaire.max_question_score.to_s + '</TD>' | |||
end | |||
html += '</TR>' | |||
Html.html_safe | |||
end | |||
</pre> | |||
* Identical code found in 1 other location: Line 135 - 139 (mass = 50) | * Identical code found in 1 other location: Line 135 - 139 (mass = 50) | ||
Also found in app/models/scale.rb:50…54. Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question | |||
<pre> | |||
# add the following functions to refactor duplicates. | |||
def complete_questionnaire_min_to_questionnaire_max(ob, html, answer, questionnaire_min, questionnaire_max) | |||
for j in questionnaire_min..questionnaire_max | |||
html += '<td width="10%"><input type="radio" id="' +j.to_s+ '" value="' +j.to_s+ '" name="Radio_' +ob.id.to_s+ '"' | |||
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j) | |||
html += '></td>' | |||
end | |||
return html | |||
end | |||
</pre> | |||
* Identical code found in 1 other location Line 13 - 14 (mass = 40) | * Identical code found in 1 other location Line 13 - 14 (mass = 40) | ||
Also found in app/models/scale.rb:10…11. Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question | |||
<pre> | |||
# add the following functions to refactor duplicates. | |||
def edit_end(ob, html) | |||
html+='<td> max_label <input size="4" value="'+ob.max_label.to_s+'" name="question['+ob.id.to_s+'][max_label]" id="question_'+ob.id.to_s+'_max_label" type="text"> min_label <input size="4" value="'+ob.min_label.to_s+'" name="question['+ob.id.to_s+'][min_label]" id="question_'+ob.id.to_s+'_min_label" type="text"></td>' | |||
html+='</tr>' | |||
end | |||
</pre> | |||
* Similar code found in 1 other location Line 103 - 105 (mass = 26) | * Similar code found in 1 other location Line 103 - 105 (mass = 26) | ||
Also found in app/models/criterion.rb:107…110. Wrap the duplicated code into an instance method | |||
<pre> | |||
def complete_drop_down_label_config(html, label) | |||
html += j.to_s | |||
html += "-" + label if label && label.length>0 | |||
html += "</option>" | |||
end | |||
</pre> | |||
* Similar code found in 2 other locations Line 152 - 153 (mass = 22) | * Similar code found in 2 other locations Line 152 - 153 (mass = 22) | ||
Also found in app/models/criterion.rb:115…116, app/models/text_area.rb:12…13. Wrap the duplicated code into an instance method | |||
<pre> | |||
def complete_answer_comment(count, html, answer) | |||
html += '<textarea cols=' +cols+ ' rows=' +rows+ ' id="responses_' +count.to_s+ '_comments" name="responses[' +count.to_s+ '][comment]" style="overflow:hidden;">' | |||
html += answer.comments if !answer.nil? | |||
end | |||
</pre> | |||
* Similar code found in 3 other locations: Line 130 - 134 (mass = 18) | * Similar code found in 3 other locations: Line 130 - 134 (mass = 18) | ||
Also found in app/models/criterion.rb:145…149, app/models/scale.rb:45…49, app/models/scale.rb:60…64. | |||
Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question | |||
<pre> | |||
# add the following functions to refactor duplicates. | |||
def complete_label(label, html) | |||
if !label.nil? | |||
html += '<td width="10%">' +label+ '</td>' | |||
else | |||
html += '<td width="10%"></td>' | |||
end | |||
end | |||
</pre> | |||
===Scale.rb=== | ===Scale.rb=== | ||
* Cyclomatic complexity for complete is too high. Method has too many lines | * Cyclomatic complexity for complete is too high. Method has too many lines | ||
<pre> | |||
def complete(count, answer=nil, questionnaire_min, questionnaire_max) | |||
html = self.txt + '<br>' | |||
html += '<input id="responses_' +count.to_s+ '_score" name="responses[' +count.to_s+ '][score]" type="hidden"' | |||
html += 'value="'+answer.answer.to_s+'"' if !answer.nil? | |||
html += '>' | |||
html += '<input id="responses_' +count.to_s+ '_comments" name="responses[' +count.to_s+ '][comment]" type="hidden" value="">' | |||
html += '<table>' | |||
html += '<tr><td width="10%"></td>' | |||
for j in questionnaire_min..questionnaire_max | |||
html += '<td width="10%"><label>' +j.to_s+ '</label></td>' | |||
end | |||
html += '<td width="10%"></td></tr><tr>' | |||
html = complete_label(self.min_label, html) | |||
html = complete_questionnaire_min_to_questionnaire_max(self, html, answer, questionnaire_min, questionnaire_max) | |||
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 = complete_label(self.max_label, html) | |||
html += '<td width="10%"></td></tr></table>' | |||
html.html_safe | |||
end | |||
</pre> | |||
* Identical code found in 1 other location: line 17-28 | * Identical code found in 1 other location: line 17-28 | ||
app/models/criterion.rb:20…32, def view_question_text. | |||
The following method is removed from subclass: scale and added to their superclass: scaled_question. | |||
<pre> | |||
#This method returns what to display if an instructor (etc.) is viewing a questionnaire | |||
def view_question_text | |||
html = '<TR><TD align="left"> '+self.txt+' </TD>' | |||
html += '<TD align="left">'+self.type+'</TD>' | |||
html += '<td align="center">'+self.weight.to_s+'</TD>' | |||
questionnaire = self.questionnaire | |||
if !self.max_label.nil? && !self.min_label.nil? | |||
html += '<TD align="center"> ('+self.min_label+') '+questionnaire.min_question_score.to_s+' to '+ questionnaire.max_question_score.to_s + ' ('+self.max_label+')</TD>' | |||
else | |||
html += '<TD align="center">'+questionnaire.min_question_score.to_s+' to '+ questionnaire.max_question_score.to_s + '</TD>' | |||
end | |||
html += '</TR>' | |||
html.html_safe | |||
end | |||
</pre> | |||
* Identical code found in 1 other location: line 50-54 | * Identical code found in 1 other location: line 50-54 | ||
app/models/criterion.rb:135…139. Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question. | |||
<pre> | |||
# add the following functions to refactor duplicates. | |||
def complete_questionnaire_min_to_questionnaire_max(ob, html, answer, questionnaire_min, questionnaire_max) | |||
for j in questionnaire_min..questionnaire_max | |||
html += '<td width="10%"><input type="radio" id="' +j.to_s+ '" value="' +j.to_s+ '" name="Radio_' +ob.id.to_s+ '"' | |||
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j) | |||
html += '></td>' | |||
end | |||
return html | |||
end | |||
</pre> | |||
* Identical code found in 1 other location: line 10-11 | * Identical code found in 1 other location: line 10-11 | ||
app/models/criterion.rb:13…14. Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question | |||
<pre> | |||
# add the following functions to refactor duplicates. | |||
def edit_end(ob, html) | |||
html+='<td> max_label <input size="4" value="'+ob.max_label.to_s+'" name="question['+ob.id.to_s+'][max_label]" id="question_'+ob.id.to_s+'_max_label" type="text"> min_label <input size="4" value="'+ob.min_label.to_s+'" name="question['+ob.id.to_s+'][min_label]" id="question_'+ob.id.to_s+'_min_label" type="text"></td>' | |||
html+='</tr>' | |||
end | |||
</pre> | |||
* Similar code found in 3 other locations: line 60-64 | * Similar code found in 3 other locations: line 60-64 | ||
app/models/criterion.rb:130…134, app/models/criterion.rb:145…149, app/models/scale.rb:45…49. | |||
Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question | |||
<pre> | |||
# add the following functions to refactor duplicates. | |||
def complete_label(label, html) | |||
if !label.nil? | |||
html += '<td width="10%">' +label+ '</td>' | |||
else | |||
html += '<td width="10%"></td>' | |||
end | |||
end | |||
</pre> | |||
===Checkbox.rb=== | ===Checkbox.rb=== | ||
* Method has too many lines. | * Method has too many lines. | ||
By moving codes to private methods outside this method, this problem can be fixed together with the following one. Here is the new complete method together with two additional private methods: hasAnswer(html, answer) and nextQuestionTail(html, next_question). The source codes are listed as below. | |||
<pre> | |||
def complete(count, answer=nil) | |||
curr_question = Question.find(self.id) | |||
prev_question = Question.where("seq < ?", curr_question.seq).order(:seq).last | |||
next_question = Question.where("seq > ?", curr_question.seq).order(:seq).first | |||
html = '' | |||
if prev_question.type == 'ColumnHeader' | |||
html = '<td style="padding: 15px;">' | |||
end | |||
html += '<input id="responses_' +count.to_s+ '_comments" name="responses[' +count.to_s+ '][comment]" type="hidden" value="">' | |||
html += '<input id="responses_' +count.to_s+ '_score" name="responses[' +count.to_s+ '][score]" type="hidden"' | |||
html += hasAnswer(html , answer) | |||
html += '>' | |||
html += '<input id="responses_' +count.to_s+ '_checkbox" type="checkbox" onchange="checkbox' +count.to_s+ 'Changed()"' | |||
html += 'checked="checked"' if !answer.nil? and answer.answer == 1 | |||
html += '>' | |||
html += '<label for="responses_' +count.to_s+ '">' +self.txt+ '</label>' | |||
html += '<script>function checkbox' +count.to_s+ 'Changed() {' | |||
html += ' var checkbox = jQuery("#responses_' +count.to_s+ '_checkbox");' | |||
html += ' var response_score = jQuery("#responses_' +count.to_s+ '_score");' | |||
html += 'if (checkbox.is(":checked")) {' | |||
html += 'response_score.val("1");' | |||
html += '} else {' | |||
html += 'response_score.val("0");}}</script>' | |||
html = nextQuestionTail(html, next_question) | |||
html.html_safe | |||
end | |||
#YJ private for complete() | |||
def hasAnswer(html, answer) | |||
if !answer.nil? and answer.answer == 1 | |||
html += 'value="1"' | |||
else | |||
html += 'value="0"' | |||
end | |||
return html | |||
end | |||
def nextQuestionTail(html, next_q) | |||
if next_question.type == 'ColumnHeader' | |||
html += '</td></tr>' | |||
elsif next_question.type == 'SectionHeader' or next_question.type == 'TableHeader' | |||
html += '</td></tr></table><br/>' | |||
else | |||
html += '<BR/>' | |||
end | |||
return html | |||
end | |||
</pre> | |||
* Cyclomatic complexity for complete is too high. | * Cyclomatic complexity for complete is too high. | ||
This problem can be solved by moving branches outside to other private methods. See source code above. | |||
* Similar codes in edit method are found in 2 other locations. | * Similar codes in edit method are found in 2 other locations. | ||
(questionnaire_header.rb and upload_file.rb) | |||
Such code duplication can be eliminated by moving the identical part of the similar edit method to a new function, named edit_prefix(html, count) to their common superclass, which is Question (question.rb). | |||
Here is the source code of the added edit_prefix method in question.rb: | |||
<pre> | |||
def edit_prefix(self, html) | |||
html ='<tr>' | |||
html+='<td align="center"><a rel="nofollow" data-method="delete" href="/questions/' +ob.id.to_s+ '">Remove</a></td>' | |||
html+='<td><input size="6" value="'+ob.seq.to_s+'" name="question['+ob.id.to_s+'][seq]" id="question_'+ob.id.to_s+'_seq" type="text"></td>' | |||
html+='<td><textarea cols="50" rows="1" name="question['+ob.id.to_s+'][txt]" id="question_'+ob.id.to_s+'_txt">'+ob.txt+'</textarea></td>' | |||
html+='<td><input size="10" disabled="disabled" value="'+ob.type+'" name="question['+ob.id.to_s+'][type]" id="question_'+ob.id.to_s+'_type" type="text">''</td>' | |||
end | |||
</pre> | |||
The original code are commented out and insert the function call of the above function, html will be returned in the above function, see codes below: | |||
<pre> | |||
def edit(count) | |||
html = edit_prefix(self, html) | |||
html+='<td><!--placeholder (UnscoredQuestion does not need weight)--></td>' | |||
html+='</tr>' | |||
html.html_safe | |||
end | |||
</pre> | |||
* Similar codes in view_question_text method are found in 4 other locations. | * Similar codes in view_question_text method are found in 4 other locations. | ||
(dropdown.rb, questionnaire_header.rb, text_response.rb, upload_file.rb) | |||
The solution to this problem is the same as above one, below is the function added in the most common superclass, which is Question, that contains the common parts of the codes found in the view_question_text methods in all five classes. | |||
<pre> | |||
def view_qt_prefix(ob, html) | |||
html = '<TR><TD align="left"> '+ob.txt+ ' </TD>' | |||
html += '<TD align="left">'+ob.type+'</TD>' | |||
html += '<td align="center">'+ob.weight.to_s+'</TD>' | |||
end | |||
</pre> | |||
The original code are commented out and insert the function call of the above function, html will be returned in the above function, see codes below: | |||
<pre> | |||
def view_question_text | |||
html = view_qt_prefix(ob, html) | |||
html += '<TD align="center">Checked/Unchecked</TD>' | |||
html += '</TR>' | |||
html.html_safe | |||
end | |||
</pre> | |||
===Questionnaire_header.rb=== | ===Questionnaire_header.rb=== | ||
All problems in this file are solved after solving the problems found in checkbox.rb. | |||
===Upload_file.rb=== | ===Upload_file.rb=== | ||
All problems in this file are solved after solving the problems found in checkbox.rb. | |||
<br> | <br> | ||
==Unit- | |||
==Unit-tests== | |||
One contribution is to write unite test for four question types. For each of the four required question types: | |||
*criterion.rb, | |||
*scale.rb, | |||
*checkbox.rb, | |||
*upload_file.rb, | |||
there are four unit tests for the following four functions: | |||
*''edit'', | |||
*''complete'', | |||
*''view_question_text'', | |||
*''view_complete_question_text''. | |||
These four functions are tested since they are the only methods that the question types class can perform. | |||
The output of those four functions is HTML format string. We test if the output string is matched with the expected given certain conditions. Here are the spec files git hub links: | |||
*[https://github.com/schen35/expertiza/blob/master/spec/models/criterion_spec.rb criterion_spec.rb] | |||
*[https://github.com/schen35/expertiza/blob/master/spec/models/scale_spec.rb scale_spec.rb] | |||
*[https://github.com/schen35/expertiza/blob/master/spec/models/checkbox_spec.rb checkbox_spec.rb] | |||
*[https://github.com/schen35/expertiza/blob/master/spec/models/upload_file_spec.rb upload_file_spec.rb]. |
Latest revision as of 01:35, 4 April 2016
This wiki page is for the description of changes made under E1608 OSS assignment for Spring 2016, CSC/ECE 517. The assignment task is to refactor & write unit tests for [question_type].rb (eg. criterion.rb).
Problem Statement
Four models is related: criterion.rb, scale.rb, checkbox.rb, questionnaire_header.rb, upload_file.rb. There are 4 methods “edit”, “view_question_text”, “complete” and “view_completed_question” correspond to different views.
- edit ~> when instructor add/edit new questions
- view ~> when instructor view existed questions
- complete ~> when students do peer reviews
- view_completed_question ~> when student view existed peer reviews
Expertiza question types inheritance hierarchy
- Choice question
- Scored question
- Scale
- Criterion
- Unscored question
- Dropdown
- CheckBox
- Scored question
- TextResponse
- TextArea
- TextField
- UploadFile
Files problem
There are some issues in each file, such as security, complexity, duplication, etc. And currently there is no unit tests for [question_type].rb.
Our contribution
- Refactor these [question_type].rb files and fix issues.
- Keep the inputs and outputs of these methods (edit, view, complete, view_completed_question) the same as before.
- Write unit tests for [question_type].rb listed above.
- Create RSpec files for each question type in /spec/models/ folder
- Create multiple tests to check valid and invalid cases, such as input including special character double quote (“”).
Refactor
Criterion.rb
- Method has too many lines: Line 35 - 158 [106/30]
Wrap the specific code into different instance methods.
- Cyclomatic complexity for complete is too high: Line 35 - 158 [28/6]
Wrap the code into different instance methods to reduce cyclomatic complexity
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 = '<li><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 { |advice| advice.id } advice_total_length = 0 question_advices.each do |question_advice| if question_advice.advice && !question_advice.advice == "" advice_total_length += question_advice.advice.length end end if question_advices.length > 0 and advice_total_length > 0 html = complete_show_advice(self, html) 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 += (question_advices.length - 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((opts - j - 1).toString());}' html += '</script>' end html += '</div>' end if dropdown_or_scale == 'dropdown' ## code added to reduce Cyclomatic Complexity html = complete_drop_down(self, html, count, answer, questionnaire_min, questionnaire_max) elsif dropdown_or_scale == 'scale' html = complete_scale(self, html, count, answer, questionnaire_min, questionnaire_max) end html.html_safe end
- Identical code found in 1 other location: Line 20 - 32 (mass = 89)
Also found in app/models/scale.rb:17…28. The following method is removed from subclass: criterion and added to their superclass: scaled_question
def view_question_text html = '<TR><TD align="left"> '+self.txt+' </TD>' html += '<TD align="left">'+self.type+'</TD>' html += '<td align="center">'+self.weight.to_s+'</TD>' questionnaire = self.questionnaire if !self.max_label.nil? && !self.min_label.nil? html += '<TD align="center"> ('+self.min_label+') '+questionnaire.min_question_score.to_s+' to '+ questionnaire.max_question_score.to_s + ' ('+self.max_label+')</TD>' else html += '<TD align="center">'+questionnaire.min_question_score.to_s+' to '+ questionnaire.max_question_score.to_s + '</TD>' end html += '</TR>' Html.html_safe end
- Identical code found in 1 other location: Line 135 - 139 (mass = 50)
Also found in app/models/scale.rb:50…54. Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question
# add the following functions to refactor duplicates. def complete_questionnaire_min_to_questionnaire_max(ob, html, answer, questionnaire_min, questionnaire_max) for j in questionnaire_min..questionnaire_max html += '<td width="10%"><input type="radio" id="' +j.to_s+ '" value="' +j.to_s+ '" name="Radio_' +ob.id.to_s+ '"' html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j) html += '></td>' end return html end
- Identical code found in 1 other location Line 13 - 14 (mass = 40)
Also found in app/models/scale.rb:10…11. Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question
# add the following functions to refactor duplicates. def edit_end(ob, html) html+='<td> max_label <input size="4" value="'+ob.max_label.to_s+'" name="question['+ob.id.to_s+'][max_label]" id="question_'+ob.id.to_s+'_max_label" type="text"> min_label <input size="4" value="'+ob.min_label.to_s+'" name="question['+ob.id.to_s+'][min_label]" id="question_'+ob.id.to_s+'_min_label" type="text"></td>' html+='</tr>' end
- Similar code found in 1 other location Line 103 - 105 (mass = 26)
Also found in app/models/criterion.rb:107…110. Wrap the duplicated code into an instance method
def complete_drop_down_label_config(html, label) html += j.to_s html += "-" + label if label && label.length>0 html += "</option>" end
- Similar code found in 2 other locations Line 152 - 153 (mass = 22)
Also found in app/models/criterion.rb:115…116, app/models/text_area.rb:12…13. Wrap the duplicated code into an instance method
def complete_answer_comment(count, html, answer) html += '<textarea cols=' +cols+ ' rows=' +rows+ ' id="responses_' +count.to_s+ '_comments" name="responses[' +count.to_s+ '][comment]" style="overflow:hidden;">' html += answer.comments if !answer.nil? end
- Similar code found in 3 other locations: Line 130 - 134 (mass = 18)
Also found in app/models/criterion.rb:145…149, app/models/scale.rb:45…49, app/models/scale.rb:60…64. Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question
# add the following functions to refactor duplicates. def complete_label(label, html) if !label.nil? html += '<td width="10%">' +label+ '</td>' else html += '<td width="10%"></td>' end end
Scale.rb
- Cyclomatic complexity for complete is too high. Method has too many lines
def complete(count, answer=nil, questionnaire_min, questionnaire_max) html = self.txt + '<br>' html += '<input id="responses_' +count.to_s+ '_score" name="responses[' +count.to_s+ '][score]" type="hidden"' html += 'value="'+answer.answer.to_s+'"' if !answer.nil? html += '>' html += '<input id="responses_' +count.to_s+ '_comments" name="responses[' +count.to_s+ '][comment]" type="hidden" value="">' html += '<table>' html += '<tr><td width="10%"></td>' for j in questionnaire_min..questionnaire_max html += '<td width="10%"><label>' +j.to_s+ '</label></td>' end html += '<td width="10%"></td></tr><tr>' html = complete_label(self.min_label, html) html = complete_questionnaire_min_to_questionnaire_max(self, html, answer, questionnaire_min, questionnaire_max) 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 = complete_label(self.max_label, html) html += '<td width="10%"></td></tr></table>' html.html_safe end
- Identical code found in 1 other location: line 17-28
app/models/criterion.rb:20…32, def view_question_text. The following method is removed from subclass: scale and added to their superclass: scaled_question.
#This method returns what to display if an instructor (etc.) is viewing a questionnaire def view_question_text html = '<TR><TD align="left"> '+self.txt+' </TD>' html += '<TD align="left">'+self.type+'</TD>' html += '<td align="center">'+self.weight.to_s+'</TD>' questionnaire = self.questionnaire if !self.max_label.nil? && !self.min_label.nil? html += '<TD align="center"> ('+self.min_label+') '+questionnaire.min_question_score.to_s+' to '+ questionnaire.max_question_score.to_s + ' ('+self.max_label+')</TD>' else html += '<TD align="center">'+questionnaire.min_question_score.to_s+' to '+ questionnaire.max_question_score.to_s + '</TD>' end html += '</TR>' html.html_safe end
- Identical code found in 1 other location: line 50-54
app/models/criterion.rb:135…139. Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question.
# add the following functions to refactor duplicates. def complete_questionnaire_min_to_questionnaire_max(ob, html, answer, questionnaire_min, questionnaire_max) for j in questionnaire_min..questionnaire_max html += '<td width="10%"><input type="radio" id="' +j.to_s+ '" value="' +j.to_s+ '" name="Radio_' +ob.id.to_s+ '"' html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j) html += '></td>' end return html end
- Identical code found in 1 other location: line 10-11
app/models/criterion.rb:13…14. Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question
# add the following functions to refactor duplicates. def edit_end(ob, html) html+='<td> max_label <input size="4" value="'+ob.max_label.to_s+'" name="question['+ob.id.to_s+'][max_label]" id="question_'+ob.id.to_s+'_max_label" type="text"> min_label <input size="4" value="'+ob.min_label.to_s+'" name="question['+ob.id.to_s+'][min_label]" id="question_'+ob.id.to_s+'_min_label" type="text"></td>' html+='</tr>' end
- Similar code found in 3 other locations: line 60-64
app/models/criterion.rb:130…134, app/models/criterion.rb:145…149, app/models/scale.rb:45…49. Wrap the duplicated code into an instance method and add the instance method into superclass: scaled_question
# add the following functions to refactor duplicates. def complete_label(label, html) if !label.nil? html += '<td width="10%">' +label+ '</td>' else html += '<td width="10%"></td>' end end
Checkbox.rb
- Method has too many lines.
By moving codes to private methods outside this method, this problem can be fixed together with the following one. Here is the new complete method together with two additional private methods: hasAnswer(html, answer) and nextQuestionTail(html, next_question). The source codes are listed as below.
def complete(count, answer=nil) curr_question = Question.find(self.id) prev_question = Question.where("seq < ?", curr_question.seq).order(:seq).last next_question = Question.where("seq > ?", curr_question.seq).order(:seq).first html = '' if prev_question.type == 'ColumnHeader' html = '<td style="padding: 15px;">' end html += '<input id="responses_' +count.to_s+ '_comments" name="responses[' +count.to_s+ '][comment]" type="hidden" value="">' html += '<input id="responses_' +count.to_s+ '_score" name="responses[' +count.to_s+ '][score]" type="hidden"' html += hasAnswer(html , answer) html += '>' html += '<input id="responses_' +count.to_s+ '_checkbox" type="checkbox" onchange="checkbox' +count.to_s+ 'Changed()"' html += 'checked="checked"' if !answer.nil? and answer.answer == 1 html += '>' html += '<label for="responses_' +count.to_s+ '">' +self.txt+ '</label>' html += '<script>function checkbox' +count.to_s+ 'Changed() {' html += ' var checkbox = jQuery("#responses_' +count.to_s+ '_checkbox");' html += ' var response_score = jQuery("#responses_' +count.to_s+ '_score");' html += 'if (checkbox.is(":checked")) {' html += 'response_score.val("1");' html += '} else {' html += 'response_score.val("0");}}</script>' html = nextQuestionTail(html, next_question) html.html_safe end #YJ private for complete() def hasAnswer(html, answer) if !answer.nil? and answer.answer == 1 html += 'value="1"' else html += 'value="0"' end return html end def nextQuestionTail(html, next_q) if next_question.type == 'ColumnHeader' html += '</td></tr>' elsif next_question.type == 'SectionHeader' or next_question.type == 'TableHeader' html += '</td></tr></table><br/>' else html += '<BR/>' end return html end
- Cyclomatic complexity for complete is too high.
This problem can be solved by moving branches outside to other private methods. See source code above.
- Similar codes in edit method are found in 2 other locations.
(questionnaire_header.rb and upload_file.rb) Such code duplication can be eliminated by moving the identical part of the similar edit method to a new function, named edit_prefix(html, count) to their common superclass, which is Question (question.rb). Here is the source code of the added edit_prefix method in question.rb:
def edit_prefix(self, html) html ='<tr>' html+='<td align="center"><a rel="nofollow" data-method="delete" href="/questions/' +ob.id.to_s+ '">Remove</a></td>' html+='<td><input size="6" value="'+ob.seq.to_s+'" name="question['+ob.id.to_s+'][seq]" id="question_'+ob.id.to_s+'_seq" type="text"></td>' html+='<td><textarea cols="50" rows="1" name="question['+ob.id.to_s+'][txt]" id="question_'+ob.id.to_s+'_txt">'+ob.txt+'</textarea></td>' html+='<td><input size="10" disabled="disabled" value="'+ob.type+'" name="question['+ob.id.to_s+'][type]" id="question_'+ob.id.to_s+'_type" type="text">''</td>' end
The original code are commented out and insert the function call of the above function, html will be returned in the above function, see codes below:
def edit(count) html = edit_prefix(self, html) html+='<td><!--placeholder (UnscoredQuestion does not need weight)--></td>' html+='</tr>' html.html_safe end
- Similar codes in view_question_text method are found in 4 other locations.
(dropdown.rb, questionnaire_header.rb, text_response.rb, upload_file.rb) The solution to this problem is the same as above one, below is the function added in the most common superclass, which is Question, that contains the common parts of the codes found in the view_question_text methods in all five classes.
def view_qt_prefix(ob, html) html = '<TR><TD align="left"> '+ob.txt+ ' </TD>' html += '<TD align="left">'+ob.type+'</TD>' html += '<td align="center">'+ob.weight.to_s+'</TD>' end
The original code are commented out and insert the function call of the above function, html will be returned in the above function, see codes below:
def view_question_text html = view_qt_prefix(ob, html) html += '<TD align="center">Checked/Unchecked</TD>' html += '</TR>' html.html_safe end
Questionnaire_header.rb
All problems in this file are solved after solving the problems found in checkbox.rb.
Upload_file.rb
All problems in this file are solved after solving the problems found in checkbox.rb.
Unit-tests
One contribution is to write unite test for four question types. For each of the four required question types:
- criterion.rb,
- scale.rb,
- checkbox.rb,
- upload_file.rb,
there are four unit tests for the following four functions:
- edit,
- complete,
- view_question_text,
- view_complete_question_text.
These four functions are tested since they are the only methods that the question types class can perform.
The output of those four functions is HTML format string. We test if the output string is matched with the expected given certain conditions. Here are the spec files git hub links: