CSC517 (Spring 2020) - E2010. Refactor criterion.rb

From Expertiza_Wiki
Jump to navigation Jump to search

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.

  • High branch condition size problem: extracting three methods from method "complete"
    • Method "dropdown_criterion_question" return html when choosing dropdown options
    • Method "scale_criterion_question" return html when choosing scale options
    • Method "advices_criterion_question" return html about showing advice for each criterion question
  • Too long code
    • Combining the short HTML strings into longer ones but not too long
    • Using one readable line of code instead of three or more lines of if-else statements
  • Fix incomplete condition problem
  • Change the language to more Ruby friendly

Implementation

We tried to tackle the issues mentioned in the problem statement as described below:

The complete method

  • We extracted three methods from "complete" method.
  • We combined the short HTML strings into longer ones.
  • We used the shorthand which beautifully consolidates three or more lines of code (read: if-else statements) into one readable line of code.

Examples of changes

  • Extracting three methods to reducing the condition branch
    • Method "dropdown_criterion_question" return html when choosing dropdown options
    • Method "scale_criterion_question" return html when choosing scale options
    • Method "advices_criterion_question" return html about showing advice for each criterion question
  • Combining the short HTML strings into longer ones

We go from: 

html += "</select></div><br><br>"
html += '<textarea' + ' id="responses_' + count.to_s + '_comments"'

To: 

html += '</select></div><br><br><textarea' + ' id="responses_' + count.to_s + '_comments"'
  • Using one readable line of code instead of three or more lines of if-else statements

We go from: 

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

To: 

html += '<option value=' + j.to_s
html += ' selected="selected"' if !answer.nil? && j == answer.answer
html += '>' + j.to_s
  • Use unless for negative conditions

We go from: 

current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?

To: 

current_value += 'data-current-rating =' + answer.answer.to_s unless answer.nil?

Same change for self.min_label and self.max_label. We go from: 

html += if !self.min_label.nil?
                '<td width="10%">' + self.min_label + '</td>'
              else
                '<td width="10%"></td>'
              end

To: 

html += '<td width="10%">'
html += self.min_label unless self.min_label.nil?
html += '</td>'
  • Unsafe condition

Add condition to make sure when answer.comments is nil. We go from: 

html += answer.comments unless answer.nil?

To: 

html += answer.comments if !answer.nil? && !answer.comments.nil?
  • Incomplete condition

When create a new criterion, which size is "" that isn't nil. We add condition to make sure "cols" and "rows" will be assign when "self.size" is "". We go from: 

if self.size.nil?
   cols = '70'
   rows = '1'
else
   cols = self.size.split(',')[0]
   rows = self.size.split(',')[1]
end
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
        ' name="responses[' + count.to_s + '][comment]" class="tinymce">'

To: 

if self.size.nil?||self.size.blank?
   cols = '70'
   rows = '1'
else
   cols = self.size.split(',')[0]
   rows = self.size.split(',')[1]
end
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
        ' name="responses[' + count.to_s + '][comment]" class="tinymce">'
  • Ruby grammer

We change "and" to &&. We changed from: 

html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)

To: 

html += 'checked="checked"' if (!answer.nil? && answer.answer == j) or (answer.nil? && questionnaire_min == j)

The modified complete method

  • Method "complete"
  def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
    html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
    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
    #show advice given for different questions
    html += advices_criterion_question(count, question_advices) if !question_advices.empty? and advice_total_length > 0

    #dropdown options to rate a project based on the quetion
    html += dropdown_criterion_question(count, answer, questionnaire_min, questionnaire_max) if dropdown_or_scale == 'dropdown'

    #scale optioins
    html += scale_criterion_question(count, answer, questionnaire_min, questionnaire_max) if dropdown_or_scale == 'scale'
    safe_join(["".html_safe, "".html_safe], html.html_safe)
  end
  • Method "advices_criterion_question"
  # show advice for each criterion question
  def advices_criterion_question(count, question_advices)
    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
  • Method "dropdown_criterion_question"
  def dropdown_criterion_question(count, answer = nil, questionnaire_min, questionnaire_max)
    current_value = "" 
    current_value += 'data-current-rating =' + answer.answer.to_s unless 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 += '<option value=' + j.to_s
      html += ' selected="selected"' if !answer.nil? && j == answer.answer
      html += '>'
      html += j.to_s
      html += "-" + self.min_label if self.min_label.present? && j == questionnaire_min
      html += "-" + self.max_label if self.max_label.present? && j == questionnaire_max
      html += "</option>"
    end

    html += '</select></div><br><br><textarea' + ' id="responses_' + count.to_s + '_comments"'
    html += ' name="responses[' + count.to_s + '][comment]" class="tinymce">'
    html += answer.comments if !answer.nil? && !answer.comments.nil?
    html += '</textarea></td>'
  end
  • Method "scale_criterion_question"
  def scale_criterion_question(count, answer = nil, questionnaire_min, questionnaire_max)
    if self.size.nil? || !self.size.present?
      cols = '70'
      rows = '1'
    else
     cols = self.size.split(',')[0]
     rows = self.size.split(',')[1]
    end
    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 += '><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%">'
    html += self.min_label unless self.min_label.nil?
    html += '</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? && answer.answer == j) or (answer.nil? && 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><td width="10%">'
    html += self.max_label unless self.max_label.nil?
    html += '</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 if !answer.nil? && !answer.comments.nil?
    html += '</textarea>'    
  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.

  • We reduced the nested if-else branching.
  • We concatenated small HTML string
  • We used shorthand for if-else condition statements.

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.

Example of changes

  • Combining the short HTML strings into longer ones

We concatenated small HTML which was from: 

html += '<table cellpadding="5">'
html += '<tr>'
html += '<td>'

To: 

html += '<table cellpadding="5"><tr><td>'
  • Using one readable line of code instead of three or more lines of if-else statements

We changed from: 

score_percent = if score != "-"
                     answer.answer * 1.0 / questionnaire_max
                else
                     0
                end

To: 

score_percent = score != "-" ? answer.answer * 1.0 / questionnaire_max : 0
  • Delete useless variable - 'resp'

We delete this code 

resp = Response.find(answer.response_id)
  • Change of language to make it more Ruby friendly

We change from: 

if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i

To: 

if tag_dep.question_type == question.type && answer.comments.length > tag_dep.answer_length_threshold.to_i
  • Reduce the branch condition

We combine condition 

unless tag_prompt_deployments.nil?

and 

if tag_prompt_deployments.count > 0

To: 

if !tag_prompt_deployments.nil? && tag_prompt_deployments.count > 0

The modified view_completed_question

  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 ####
      if !tag_prompt_deployments.nil? && tag_prompt_deployments.count > 0
        # show check boxes for answer tagging
        question = Question.find(answer.question_id)
        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 && 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 code to show tag prompts ####
    end
    html += '</tr></table>'
    safe_join(["".html_safe, "".html_safe], html.html_safe)
  end

Testing

We are running Rspec tests to make sure the coverage is the same and manually checking the pages to make sure there are not any issues and the code is not breaking

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC517_(Spring_2020)_-_E2010._Refactor_criterion.rb&oldid=132255"