View source for CSC517 (Spring 2020) - E2010. Refactor criterion.rb

= E2010 Refactor criterion.rb =

This page provides a description of the Expertiza based OSS project.

== About Expertiza ==

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.
<pre>
  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
</pre>
* 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.
<pre>
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
</pre>


== 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 ===
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. 

====''Examples of changes''====

* '''Combining the short HTML strings into longer ones'''
We go from:
<pre>
html += "</select></div><br><br>"
html += '<textarea' + ' id="responses_' + count.to_s + '_comments"'
</pre>
To:
<pre>
html += '</select></div><br><br><textarea' + ' id="responses_' + count.to_s + '_comments"'
</pre>
* '''Using one readable line of code instead of three or more lines of if-else statements'''
We go from:
<pre>
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
</pre>
To:
<pre>
html += '<option value=' + j.to_s
html += ' selected="selected"' if !answer.nil? && j == answer.answer
html += '>' + j.to_s
</pre>
* '''Use unless for negative conditions'''
We go from:
<pre>
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?
</pre>
To:
<pre>
current_value += 'data-current-rating =' + answer.answer.to_s unless answer.nil?
</pre>

Same change for self.min_label and self.max_label. We go from:
<pre>
html += if !self.min_label.nil?
                '<td width="10%">' + self.min_label + '</td>'
              else
                '<td width="10%"></td>'
              end
</pre>
To:
<pre>
html += '<td width="10%">'
html += self.min_label unless self.min_label.nil?
html += '</td>'
</pre>

* '''Unsafe condition for dropdown and scale'''
Add condition to make sure when answer.comments is nil. We go from:
<pre>
html += answer.comments unless answer.nil?
</pre>
To:
<pre>
html += answer.comments if !answer.nil? && !answer.comments.nil?
</pre>

* '''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:
<pre>
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">'
</pre>
To:
<pre>
if self.size.nil?||!self.size.present?
   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">'
</pre>

* '''Ruby grammer'''
We change "and" to && We go from:
<pre>
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
</pre>
To:
<pre>
html += 'checked="checked"' if (!answer.nil? && answer.answer == j) or (answer.nil? && questionnaire_min == j)
</pre>

====''The modified ''complete'' method''====

<pre>
  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

  # 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

  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

  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
</pre>

=== 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. 

====''Example of changes''====
* '''Combining the short HTML strings into longer ones'''

  [[File:Combine2_1.png]]

* '''Using one readable line of code instead of three or more lines of if-else statements'''

  [[File:Oneline2_1.png]]

* '''Delete useless variable - 'resp''''
We delete this code
<pre>
resp = Response.find(answer.response_id)
</pre>

* '''Change of language to make it more Ruby friendly'''
We change from:
<pre>
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
</pre>
To:
<pre>
if tag_dep.question_type == question.type && answer.comments.length > tag_dep.answer_length_threshold.to_i
</pre>
* '''Reduce the branch condition'''
We combine condition 
<pre>
unless tag_prompt_deployments.nil?
</pre>
and
<pre>
if tag_prompt_deployments.count > 0
</pre>
To:
<pre>
if !tag_prompt_deployments.nil? && tag_prompt_deployments.count > 0
</pre>
  [[File:Multiple_2.png]]

==== The modified ''view_completed_question''====

<pre>
  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
</pre>

== 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