CSC/ECE 517 Fall 2014/oss E1457 ags

Expertiza - Refactoring DynamicReviewMapping Controller and the ReportsController

Introduction

It is time consuming to assess project work in a large classroom environment, and one of the best techniques to achieve this is to have students assist in this assessment. If each student is asked to assess a few (say, one to five) other students or student teams, the task requires a reasonable amount of effort; and that effort does not increase as the class gets larger. "Expertiza is a system for managing all kinds of communication that is involved in assessment: double-blind communications between authors and reviewers, assessment of teammate contributions, evaluations by course staff, and surveys of students to assess the assignment and the peer-review process." <ref>http://www.igi-global.com/book/monitoring-assessment-online-collaborative-environments/773&f=e-book</ref> "Expertiza project uses peer review to create reusable learning objects."<ref>http://expertiza.ncsu.edu/</ref> Students select from a list of tasks to be performed, with several students selecting each task. Then they prepare their work and submit it to an electronic peer-review system. The work is then reviewed by other students, who offer comments to help the submitters improve their work. The best submissions for each task are then selected for use in later courses. Expertiza gets students working together to improve others’ learning experiences. It helps them learn, by making them think through the lecture material and apply it to a real-world situation, just as they might do in an on-the-job situation. These learning objects can be improved iteratively; for example, the next year’s class can be presented with the examples developed by students the previous year, and asked to identify shortcomings and develop improved examples.<ref>http://scholar.google.com/scholar?q=%22Reusable+learning+objects+through+peer+review%3A+The+Expertiza+approach%22+%22The+Expertiza+platform+is+a+divide-and-conquer%22&btnG=&hl=en&as_sdt=0%2C34</ref> More information on Expertiza can be found here.

Refactoring in Ruby

Refactoring helps to<ref>http://www.refactoringinruby.com/</ref>

• Understand what led to poor code design
• Fix code which is difficult to understand
• Express each idea “once and only once”
• Recognize missing or inadequately formed classes
• Simplify overly complex relationships between classes
• Achieve the right balance of responsibilities among objects
• Make code easier to test and more maintainable

Code Smells

We identified the following categories of code smells in the helper module:

• Lengthy Definitions: These are the methods that have too many responsibilities and collaborators. As a result of which, their overall length has grown too long reducing the maintainability and readability.

• Duplicated Code: These are those pieces of code that does almost the same functionality of some other piece of code leading to a bad design style. They are usually too error prone since not everyone identifies all the code responsible for the same task and does the changes whenever the need arises.

• Bad method names: A good method name is expected to adhere to the Ruby style and design style guidelines. It is expected to convey a reasonable amount of responsibilities handled. A good method name in combination with good practices should resemble a user story.

• Used and unnecessary methods: These are those methods that were useful when the code was first developed. But, they lost their functionality as the code got updated and new functionalities were added or existing ones were modified.

Refactoring Techniques

There are many documented refactoring techniques, and a few common ones are below.<ref>http://www.integralist.co.uk/posts/refactoring-techniques/</ref>

• Rename Method: Renaming identifiers can reduce the need for code comments and nearly always helps to promote greater clarity.
• Introduce Explaining Variable: So here is a technique specifically based around the premise of renaming.

For example:

unless "This is a String with some CAPS".scan(/([A-Z])/).empty?
  puts "capitalised text was found"
end

Should be:

caps_not_found = "This is a String with some CAPS".scan(/([A-Z])/).empty?

unless caps_not_found
  puts "capitalised text was found"
end

• Extract Method: It consists of breaking up long methods by shifting overly complex chunks of code into new methods which have very descriptive identifiers.

For example:

class Foo
  attr_reader :bar

  def initialize bar
    @bar = bar
  end

  def do_something
    puts "my baz" # notice this is duplication
    puts bar
  end

  def do_something_else
    puts "my baz" # notice this is duplication
    puts "Something else"
    puts bar
  end
end

Becomes:

class Foo
  attr_reader :bar

  def initialize bar
    @bar = bar
  end

  def do_something
    baz
    puts bar
  end

  def do_something_else
    baz
    puts "Something else"
    puts bar
  end

  def baz
    puts "my baz"
  end
end

• Inline Method: Sometimes you want the opposite of the Extract Method technique. Imagine a method exists whose content is already simple and clear, and whose identifier adds no extra benefit. So to fix this problem we'll convert the method invocation into an inlined piece of code.

• Pull Up Method: When you have duplicated code across two separate classes then the best refactoring technique to implement is to pull that duplicate code up into a super class so we DRY (Don't Repeat Yourself) out the code and allow it to be used in multiple places without duplication (meaning changes in future only have to happen in one place).

For example:

class Person
  attr_reader :first_name, :last_name

  def initialize first_name, last_name
    @first_name = first_name
    @last_name = last_name
  end

end

class MalePerson < Person
  # This is duplicated in the `FemalePerson` class
  def full_name
    first_name + " " + last_name
  end

  def gender
    "M"
  end
end

class FemalePerson < Person
  # This is duplicated in the `MalePerson` class
  def full_name
    first_name + " " + last_name
  end

  def gender
    "F"
  end
end

Becomes:

class Person
  attr_reader :first_name, :last_name

  def initialize first_name, last_name
    @first_name = first_name
    @last_name = last_name
  end

  def full_name
    first_name + " " + last_name
  end
end

class MalePerson < Person
  def gender
    "M"
  end
end

class FemalePerson < Person
  def gender
    "F"
  end
end

• Introduce Named Parameter:When method arguments are unclear then convert them into named parameters so they become clearer (and easier to remember).

Project Description

We were supposed to refactor two controllers, the ReportsController and the DynamicReviewMapping Controller. The ReportsController was thought to be generating reports for students, showing all their scores in a single page. The DynamicReviewMapping Controller is actually a helper to the ReviewMapping controller. The review mapping controller maps reviews to each user. Currently, there are two ways to assign reviews to user: The user can either select a submission himself, which he wants to review, or the site can suggest a review to the user. Currently, the user can give more than two reviews. The ReviewMapping controller had a method so that all the users are assigned reviews automatically. Originally, the plan was to assign only a fixed number of reviews to each user. The dynamic review controller helped the review mapping controller assign reviews so that the reviews are never mapped such that the last user does not have any review to map, except his own. For example, consider three users, U1, U2 and U3. If U1 is assigned U2's work and U2 is assigned U1's work, U3 would not have any work to review. The dynamic review mapping controller makes sure that such a condition does not occur.

We were supposed to refactor these controllers. The ReportsController did not have any known use, and the DynamicReviewMapping Controller had changes to be made to make it conform to standards.

Modification to Existing Code

ReportsController

After searching through various controllers, looking for references used in the controller, we figured out that the code in the ReportsController was actually copied from the ResponseController, in December 2013. The ResponseController has been refactored since, and thus is an updated version. As of the usage, only the ResponseController has been referenced, and the ReportsController had never been used. We also extracted an older version of the ReportsController, that is, the one before it was replaced with code from the ResponseController. The code had a single method, and the method did not have any useful functionality. It was, thus, decided that the ReportsController should be removed from the Expertiza project, and that removing it would not have any effect on the overall project. Keeping it in would, on the other hand, create confusion for other people who would work on the project later.

DynamicReviewMapping Controller

• The code block between line 808 to 825 was duplicated in 3 places in this file. We have refactored the code to remove this bad smell. We have created a method called create_message to avoid duplication in the code. (Violated the DRY Principle)

• Method assign_metareviewers, assign_reviewers_team were too long. We have made two new methods (check_assignment_for_review and show_message_for_review_count) that does just one task and does it perfectly instead of large methods to do multiple tasks. (Violated the Single Responsibility Principle and the DRY Principle. Eliminated lengthy methods and meaningless method names)

• Method assign_reviewers_individual was not used anymore. We have checked for its functionality and deleted it since it did not play any role in the final application. (Eliminated unnecessary methods)

• We have changed if to unless wherever necessary.

• Changed " == 0" expression to ".zero?"

• Used if (var) instead of if (var == true)

• We have used array checking and made changes according to the following rules:

• • Use [].empty? instead of [].length == 0 or [].length.zero?
  • Use [:foo].any? instead of [:foo].length > 0
  • Use [:foo].one? instead of [:foo].length == 1
  • Use [:foo].first instead of [:foo][0]
  • Use [:foo].last instead of [:foo][-1]

• We have used && and || rather than and and or to keep boolean precedence.

None of this would actually change the functionality of the code. To make sure, we tried to look for the usage of the code. Again, after extensive searching, we figured that the code was never used. The DynamicReviewMapping controller, as stated above, depended on a set of rules that were not applicable anymore. The controller exists so that it is available, in case the rules are reverted to the original one. As such, currently the Controller is not used. We made the changes, but we have no way to check the code. We did the best we could, and we are sure that there are no errors in the refactored code.

Further Reading

• Refactoring In Ruby
• Refactoring

References

<references/>