E1911 Refactor Criterion

E1911 Refactoring 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 bulk of the code in criterion is HTML. It is the violation of MVC architecture - Model should not concern itself with how the data is displayed. This code needs to be moved to a partial file, and the partial file needs to be called in all appropriate places which call the criterion’s model methods. Once the logic for the view is moved out of the model, the model should only be left with business logic. This business logic code can also be refactored. Plus there are virtually no comments, properly comment the code.

About Criterion.rb

Criterion is one of the types of questions that can be added to a questionnaire. There are many other types of question objects like dropdown that have the implementation of similar methods. As of now, the criterion model holds 4 methods that are called from different places in the application. Each of these methods is storing a string value which contains the necessary HTML lines to display the required functionalities to the user. This string value is then returned to the calling methods from the respective views. This HTML string is then entered wherever necessary. This is ideally the exact function of a partial, and this page needs to be heavily reformatted to use partials instead of returning a string containing html code.

The pull request

https://github.com/expertiza/expertiza/pull/1407

The edit method

This method is one of the several other methods in other files like criterion.rb which gets called when the type of the question being created in the respective model. In the case of this problem, which deals with criterion type questions, the edit method defined in criterion.rb is called when the question.type is equal to "Criterion". This method allows for a user to create the criterion question and enter the necessary details. This method is called only from two view files - one for creating (_questionnaire.html.erb partial file) and another for editing (edit.html.erb). Both these files are in questionnaires view. Verified this using both RubyMine and the grep command. Another interesting thing to note is the use of "self." to get the attributes associated with question object. When we move this code to a partial, we need to change this in the erb file. The outcome of this method's refactoring will completely remove this method from the model file criterion.rb as there is no business logic involved here.

def edit(_count)
    html = '<td align="center"><a rel="nofollow" data-method="delete" href="/questions/' + self.id.to_s + '">Remove</a></td>'

    html += '<td><input size="6" value="' + self.seq.to_s + '" name="question[' + self.id.to_s + '][seq]"'
    html += ' id="question_' + self.id.to_s + '_seq" type="text"></td>'

    html += '<td><textarea cols="50" rows="1" name="question[' + self.id.to_s + '][txt]"'
    html += ' id="question_' + self.id.to_s + '_txt" placeholder="Edit question content here">' + self.txt + '</textarea></td>'

    html += '<td><input size="10" disabled="disabled" value="' + self.type + '" name="question[' + self.id.to_s + '][type]"'
    html += ' id="question_' + self.id.to_s + '_type" type="text"></td>'

    html += '<td><input size="2" value="' + self.weight.to_s
    html += '" name="question[' + self.id.to_s + '][weight]" id="question_' + self.id.to_s + '_weight" type="text"></td>'

    html += '<td>text area size <input size="3" value="' + self.size.to_s
    html += '" name="question[' + self.id.to_s + '][size]" id="question_' + self.id.to_s + '_size" type="text"></td>'

    html += '<td> max_label <input size="10" value="' + self.max_label.to_s + '" name="question[' + self.id.to_s
    html += '][max_label]" id="question_' + self.id.to_s + '_max_label" type="text">  min_label <input size="12" value="' + self.min_label.to_s
    html += '" name="question[' + self.id.to_s + '][min_label]" id="question_' + self.id.to_s + '_min_label" type="text"></td>'

    safe_join(["<tr>".html_safe, "</tr>".html_safe], html.html_safe)
  end

The safe_join is later used to return the HTML string to the calling view.

The view_question_text method

This method has the same explanation as the edit method except the fact that this method is called only during viewing of questionnaires by an instructor. We observed that this method is also called by in student_quiz view. On further analysis, we found that the student_quiz does not use criterion object and has only three options: TrueFalse, MultipleChoiceRadio and MultipleChoiceCheckbox. So no refactoring is required here. Verified this method isn't called anywhere else using RubyMine and grep command. The outcome of this method's refactoring will completely remove this method from the model file criterion.rb as there is no business logic in this method and is completely html code.

  def view_question_text
    html = '<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
      html += ' 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

    safe_join(["<TR>".html_safe, "</TR>".html_safe], html.html_safe)
  end

The complete method

This method is one of the several other methods in other files like criterion.rb which gets called when the user responds to the question being created in the respective model. In the case of this problem, which deals with criterion type question responses, the complete method defined in criterion.rb is called when the question.type is equal to "Criterion". This method allows for a user to respond to the criterion question and enter the necessary details. This method is called only from one view file i.e. response.html.erb. This file is in response view. Verified this using both RubyMine and the grep command. Unlike edit or view_question_text the self object does not refer to a global object instead it refers to a copy of the local object. So, we had passed the required object as locals to the partial, along with the parameters required by the method namely question, count, answer, questionnaire_min, questionnaire_max, dropdown_or_scale. When we move this code to a partial, we need to change this in the erb file. The outcome of this method's refactoring will completely remove this method from the model file criterion.rb as there is no business logic involved here.

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

No changes were made to this method as this method wasn't being called from a view. Instead, it was being called by a model response.rb which was also creating an html string. We think it's best to refactor this method along with response.rb as refactoring this method without refactoring response.rb would make the code messier. More explanation is given in the next section.

Proposed Solution

All these methods contain HTML text. What we propose to do is to pick these lines of code and move them into an appropriate partial, in the required format. The next task is to find out where in the entire application do these methods get called. The multiple overriding of the method calls in this poorly structured application makes it a challenging task. These method-calls then have to be replaced with the appropriate rendering of a partial in the views. This would make the structure more MVC oriented, and help keep it clean and understandable for the next developer who accesses this file.

Implementation

The changes proposed above were implemented as described below.

The Edit Method

All the code in the model was just html and thus was completely removed from there. It was moved to the partial file (views/questionnaires/_criterion_edit.html.rb) and the necessary changes were made. Since it was all HTML code, no comments were actually required. However, we still added comments describing the partial file itself. Other comments were added whenever changes were made.

Affected View Files

1. views/questionnaires/edit.html.erb - This file calls the edit method for all types of question objects and the object type takes care of invoking the right overridden method. We modified this file to call the partial instead of edit method only for Criterion type of question objects. This view is invoked during edit on the questionnaire. Here are the changes:

<% for @question in @questionnaire.questions %>
-   <%-# The following line call certain method of the object, which returns html string-%>
+   <%# E1911: Call the criterion_edit partial if question is of type Criterion -%>
+   <% if @question.is_a? Criterion %>
+     <%= render :partial => 'criterion_edit' %>
+   <% else %>
      <%=@question.edit%>
+ <% end %>
...

2. views/questionnaires/_questionnaire.html.erb - This partial file is called by new.html.erb when anew question is created. Same logic and changes as above.

3. views/questionnaires/_criterion_edit.html.erb - This file has all the html code that was earlier in the edit method of the criterion model. We changed the variables from "self.xxx" to "@question.xxx" because self was referring to question object in this context. Apart from this we changed the code to erb format. No more changes were deemed necessary. Please refer to the pull request to refer to the changes in this file as they are pure html and really long to be put in this wiki.

The view_question_text Method

This method did not contain any business logic and thus all code was formatted and moved to a new partial file (views/questionnaires/_criterion_view.html.erb). Again, no comments were required due to complete html code and a comment describing the partial was added in addition to other necessary comments in other files.

Affected View Files

1. views/questionnaires/view.html.erb - Modified to invoke the partial if question is of type Criterion only. Also changed the question variable to a instance variable (question => @question) to extend it's scope to the partial. This view is invoked during the view of questions in a questionnaire. Here are the changes:

- <%questions.each do |question| %>
-   <%if question.is_a? Question%>
-     <%=question.view_question_text.html_safe%>
+ <% for @question in questions %>
+   <%# E1911: Call the criterion_view partial if question is of type Criterion %>
+   <%if @question.is_a? Criterion %>
+         <%= render :partial => 'criterion_view' %>
+     <%elsif @question.is_a? Question%>
+         <%= @question.view_question_text.html_safe %>
    <%end%>

2. views/questionnaires/_criterion_view.html.erb - This partial file has the moved html code from view_question_text. Note that the "self.xxx" have been updated to "@question.xxx" as self referred to question object discussed in 1. Comment added to describe the partial file. Refer the pull request to look at the code.

The complete Method

All the code in the model included html thus was completely removed from there. It was moved to the partial file (app/views/response/_criterion_complete.html.erb) and the necessary changes were made. Since it was all HTML code, no comments were actually required. However, we still added comments describing the partial file itself. Other comments were added whenever changes were made.

Affected View Files

1. views/repsonse/response.html.erb - This file calls the complete method for all types of question objects and the object type takes care of invoking the right overridden method. We modified this file to call the partial instead of the complete method only for Criterion type of question objects. This view is invoked during user responds to the questionnaire. Here are the changes:

<% elsif question.instance_of? Scale %>
<!--E1911: Replaced the call to criterion.complete method with the newly refactored partial-->
<%= render partial: 'criterion_complete', :locals => {:question => question, :count => i, :answer => answer, :questionnaire_min => @questionnaire.min_question_score, :questionnaire_max => @questionnaire.max_question_score, 
:dropdown_or_scale => @dropdown_or_scale} %>

<% elsif question.instance_of? Scale %>
...

2. views/response/_criterion_complete.html.erb - This file has all the html code that was earlier in the complete method of the criterion model. We changed the variables from "self.xxx" to "question.xxx" because self was referring to copy of question object in this context. Apart from this we changed the code to erb format. No more changes were deemed necessary. Please refer to the pull request to refer to the changes in this file as they are pure html and really long to be put in this wiki.

The view_completed_question Method

This method returns a constructed html string that is used to display a particular criterion question. Although this is similar to the other 3 methods, the html variable returned from this function is not directly called into a view. If that was the case, this could be simply moved into a partial as well.

Instead, this html string is appended to another larger string variable named code in the response model, which builds the entire webpage by iterative appending. The html code for a criterion question being completed is a mere "part" of this entire string, which also includes other things such as the navbar of the page, links to the buttons, other questions, the header and the footer and such.

def add_table_rows questionnaire_max, questions, answers, code, tag_prompt_deployments = nil, current_user = nil
    count = 0
    # loop through questions so the the questions are displayed in order based on seq (sequence number)
    questions.each do |question|
      count += 1 if !question.is_a? QuestionnaireHeader and question.break_before == true
      answer = answers.find {|a| a.question_id == question.id }
      row_class = count.even? ? "info" : "warning"
      row_class = "" if question.is_a? QuestionnaireHeader
      code += '<tr class="' + row_class + '"><td>'
      if !answer.nil? or question.is_a? QuestionnaireHeader
        code += if question.instance_of? Criterion
                  #Answer Tags are enabled only for Criterion questions at the moment.
                  question.view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments, current_user) || ''
                elsif question.instance_of? Scale
                  question.view_completed_question(count, answer, questionnaire_max) || ''
                else
                  question.view_completed_question(count, answer) || ''
                end
      end
      code += '</td></tr>'
    end
    code
  end

This "code" variable is then used to display the entire page through the view method under response.rb.

A partial cannot be rendered halfway through the computation of "code", neither can it be called from a model. Since code is containing the html string, and not erb, we cannot append the render partial line to the code variable as well.

To refactor this method, we need to first refactor the entire response.rb to not append the html string for an entire webpage in a single variable, but rather use appropriate views to render the webpage.

Refactoring response model would require changes to other question subclasses like scale along with changes to the model itself. We think that it is best to refactor this method when response model is refactored along with other affected subclasses. Also, refactoring an entire model would be beyond the scope of this project as the scale of the task is that of an entire project by itself.

Thus, this method has been left untouched and still remains in criterion.rb itself.

Testing

Since our project only dealt with refactoring existing code, there was no particular need to test any specific components of the application. That being said, there were previous tests that were testing if the html variable returned by those methods was in the expected format or not.

  describe "#edit" do
    it "returns the html " do
      html = criterion.edit(0).to_s
      expect(html).to eq("<tr><td align=\"center\"><a rel=\"nofollow\" data-method=\"delete\" href=\"/questions/1\">Remove</a></td><td>
<input size=\"6\" value=\"1.0\" name=\"question[1][seq]\" id=\"question_1_seq\" type=\"text\"></td><td><textarea cols=\"50\" rows=\"1\" 
name=\"question[1][txt]\" id=\"question_1_txt\" placeholder=\"Edit question content here\">test txt</textarea></td><td><input size=\"10\"
 disabled=\"disabled\" value=\"Criterion\" name=\"question[1][type]\" id=\"question_1_type\" type=\"text\"></td><td><input size=\"2\" 
value=\"1\" name=\"question[1][weight]\" id=\"question_1_weight\" type=\"text\"></td><td>text area size <input size=\"3\" value=\"\" 
name=\"question[1][size]\" id=\"question_1_size\" type=\"text\"></td><td> max_label <input size=\"10\" value=\"\" name=\"question[1]
[max_label]\" id=\"question_1_max_label\" type=\"text\">  min_label <input size=\"12\" value=\"\" name=\"question[1][min_label]\" 
id=\"question_1_min_label\" type=\"text\"></td></tr>")
    end
  end

  describe "#view_question_text" do
    it "returns the html " do
      html = criterion.view_question_text.to_s
      expect(html).to eq("<TR><TD align=\"left\"> test txt </TD><TD align=\"left\">Criterion</TD><td align=\"center\">1</TD><TD align=\"center\"> () 0 to 5 ()</TD></TR>")
    end
  end

  describe "#complete" do
    it "returns the html " do
      html = criterion.complete(0, nil, 0, 5).to_s
      expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div>")
    end
  end

Now that these methods do not exist, there is no need to keep these test files as there is no html variable being returned or parsed. Therefore, we have removed these redundant test cases.