CSC/ECE 517 Fall 2016/E1674.Refactor leaderboard.rb and write unit tests

This page give a description of the changes made for the leaderboard.rb of Expertiza based OSS project.

Expertiza Background

Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). Instructor adds and edits assignment to Expertiza. And students can be assigned in team based on their selection of the topics. The Expertiza project is supported by the National Science Foundation.

Problem Statement

The task of the project is to refactor leaderboard.rb and write unit tests for it. As the names of the methods are not in a standard style, so we used snake_case for method names. There are also some useless assignments to several variables. The code climate rating of leaderboard.rb is D, which is needed to be improved.

Modified File

• leaderboard.rb
• create a RSpec file in /spec/models/ foler which contains unit test for leaderboard.rb

Thoughts

What we need to do is to deal with the problems listed by the code climate. And we also need to write a new test file to test the functionality of leaderboard.rb. However, the leaderboard itself has some problems and it will report some problems when we want to run it. So we need to know which method in this file is broken first and just test others instead.

Tools

As for this project, we used the code climate plugin to test if our code is good enough. This plugin will obviously help us with the refactor part because it can show which part of the original codes have some problems. We also searched through the internet and get to know how to write the Rspec files.

Refactor

We will list each kind of problems we solved below.

Use snake_case for method names.

As we all know the snake_case means we need to name the methods and variables in this way. For example, we changed this method.

 def self.getIndependantAssignments(user_id)
   assignmentIds = AssignmentParticipant.where(user_id: user_id).pluck(:parent_id)
 end

Both variable names and method names are not in a good manner. We changed it to the version below.

 def self.get_independant_assignments(user_id)
   assignment_ids = AssignmentParticipant.where(user_id: user_id).pluck(:parent_id)
 end

Useless assignment to variable - `noCourseAssignments`.

We just commented these useless variables.

Useless assignment to variable - `assignmentList`.

We just comment this useless variable.

Prefer `each` over `for`.

We know that 'each' function is more in ruby style for loops. So we would like to change all our 'for' to 'each'.

 for scoreEntry in scores
    ...
 end

We changed this loop to:

 scores.each do |scoreEntry|
    ...
 end

And the problem would be fixed.

Avoid more than 3 levels of block nesting.

 if qTypeHash.fetch(questionnaireType, {}).fetch(courseId, nil).nil?
    if qTypeHash.fetch(questionnaireType, nil).nil?
       ...
    end
    ...
 end

This is the third and fourth level of the block nesting. We need to merge them together.

 if q_type_hash.fetch(questionnaire_type, {}).fetch(course_id, nil).nil? && q_type_hash.fetch(questionnaire_type, nil).nil?
    ...
 elseif  q_type_hash.fetch(questionnaire_type, {}).fetch(course_id, nil).nil? && !q_type_hash.fetch(questionnaire_type, nil).nil?
    ...
 end

Then the 4 levels loop has been changed to 3 levels.

`end` at 148, 1 is not aligned with `def` at 129, 2.

We just need to add a space in 148 line.

Additional

There are still some problems we need to figure out. For the code revising part, there are some methods with this problem: Assignment Branch Condition size for get_participants_score is too high.

I think we cannot make too many functions in one method so that we need to make some of these be executed outside of this method. Also, some of the code lines are too long so that it is not clear enough.