CSC/ECE 517 Fall 2014/OSS E1464 vnn

E1464: Refactoring User model

The requirements provided for the Open Source System project of Expertiza were as follows :
Classes:
user.rb (295 lines)

What they do:
This model helps the user log in and checks whether the user is a student, admin or a super admin. It also helps reset passwords and assign instrcutors to the user.

What is needed:

• Writing proper getters/setters if necessary, otherwise remove getters/setters
• Replacing inefficient boolean logic (`if true` is not conditional)
• Replacing old-style ActiveRecord queries
• Using {} and [] instead of Hash.new and Array.new
• Not instantiating more than 1 instance variable per action

Introduction to Expertiza

Expertiza is a project developed using Ruby on Rails platform. It provides features like peer review, team assignments and submission of projects. This can be achieved by submitting code base, URL of hosted code on remote server and Wiki submissions. It is open source application and the code can be cloned from github. This application provides an efficient way to manage assignments, grades and reviews. This makes the process easier and faster when the class strength is large.

Expertiza is supported by National Science Foundation under Grant No. 0536558. Additional funding from the NCSU Learning in a Technology-Rich Environment (LITRE) program, the NCSU Faculty Center for Teaching and Learning, the NCSU STEM Initiative, and the Center for Advanced Computing and Communication.

Project Resources

GitHub Project Repository

VCL IP Address

Expertiza Wiki

Background

Any website that wants to provide content or features based on user preferences/roles needs to handle users. A website can do this by authenticating its users by a login/verification process.

Expertiza does this with the help of the User Model(user.rb). Expertiza has multiple user roles like Student, Instructor, Admin, Super Admin and TA. User Model helps the user login and verify to access privileges for each user.

The User Model(user.rb) also handle various other features like user access management (for example, Setting Instructors, TAs for a set of Students), user password management (for example, forgot password feature, generating random password) etc.

Code Issues

We have identified quite a few issues in the User Model of the Expertiza project. Given below are the issues that we have identified (and fixed thereafter):

1. Inefficient boolean logic present at multiple lines in the code
2. Not using [ ] & { } instead of Array.new & Hash.new (for reasons like code hygiene and efficiency)
3. Using “key => value” instead of “key: value”
4. Not using in-built array functions such as one?, any?, zero?, first & last
5. Methods that were not being used
6. Methods that were just delegating their implementations to other functions in other classes

Code Changes

For each of the above stated issues, we have made the following changes to improve the code:

Inefficient boolean logic present at multiple lines in the code

Before Changes

if elt==true
  yes
elsif elt ==false 

After Changes

if elt
  yes
elsif !elt

Not using [ ] & { } instead of Array.new & Hash.new (for reasons like code hygiene and efficiency)

Before Changes

tcsv = Array.new

After Changes

tcsv = []
fields = Array.new
was changed to
fields = []

Using “key => value” instead of “key: value”

Before Changes

has_many :participants, :class_name => 'Participant', :foreign_key => 'user_id', :dependent => :destroy
has_many :assignment_participants, :class_name => 'AssignmentParticipant', :foreign_key => 'user_id', :dependent => :destroy
has_many :assignments, :through => :participants

After Changes

has_many :participants, class_name: 'Participant', foreign_key: 'user_id', dependent: :destroy
has_many :assignment_participants, class_name: 'AssignmentParticipant', foreign_key: 'user_id', dependent: :destroy
has_many :assignments, through: :participants

Before Changes

has_many :teams_users, :dependent => :destroy
has_many :teams, :through => :teams_users

After Changes

has_many :teams_users, dependent: :destroy
has_many :teams, through: :teams_users

Before Changes

has_many :children, class_name: 'User', :foreign_key => 'parent_id'

After Changes

has_many :children, class_name: 'User', foreign_key: 'parent_id'

Before Changes

validates_presence_of :email, :message => "can't be blank"
validates_format_of :email, :with => /\A[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,4}\z/i, :allow_blank => true

After Changes

validates_presence_of :email, message: "can't be blank"
validates_format_of :email, :with => /\A[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,4}\z/i, allow_blank: true

Before Changes

all_users = User.all(:conditions => ['name LIKE ?', "#{name}%"], :limit => 20) # higher limit, since we're filtering

Before Changes

all_users = User.all(conditions: ['name LIKE ?', "#{name}%"], limit: 20) # higher limit, since we're filtering
@courses = Course.where(instructor_id: self.id, :order => 'name')

After Changes

@courses = Course.where(instructor_id: self.id, order: 'name')

Before Changes

newuser = url_for :controller => 'users', :action => 'new'

After Changes

newuser = url_for controller: 'users', action: 'new'

Not using in-built array functions such as one?, any?, zero?, first & last

Before Changes

if userList != nil && userList.length == 1

After Changes

if userList != nil && userList.one?

Methods that were not being used

Before Changes

def salt_first?
  true
end

After Changes
was commented out

#def salt_first? //Commenting the function as it is not being called anywhere
#  true
#end

Methods that were just delegating their implementations to other functions in other classes

Before Changes

def admin?
  role.admin?
end
def student?
role.student?
end

After Changes
was commented out and delegated using the code

delegate :admin?, :student?, to: :role