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

  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

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.