CSC/ECE 517 Spring 2016/Refactor and write unit tests for question type.rb

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

Files related

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
• 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.
• Cyclomatic complexity for complete is too high.
• Similar codes in edit method are found in 2 other locations.
• Similar codes in view_question_text method are found in 4 other locations.