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:

1. Inefficient boolean logic present at multiple lines in the code

Initial code: if elt==true

 yes

elsif elt ==false was changed to if elt

 yes

elsif !elt

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

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

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

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 was changed to 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 has_many :teams_users, :dependent => :destroy has_many :teams, :through => :teams_users was changed to has_many :teams_users, dependent: :destroy has_many :teams, through: :teams_users has_many :children, class_name: 'User', :foreign_key => 'parent_id' was changed to has_many :children, class_name: 'User', foreign_key: 'parent_id' 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 was changed to 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 all_users = User.all(:conditions => ['name LIKE ?', "#{name}%"], :limit => 20) # higher limit, since we're filtering was changed to 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') was changed to @courses = Course.where(instructor_id: self.id, order: 'name') newuser = url_for :controller => 'users', :action => 'new' was changed to newuser = url_for controller: 'users', action: 'new'

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

if userList != nil && userList.length == 1 was changed to if userList != nil && userList.one?

1. Methods that were not being used

def salt_first?

 true

end was commented out

1. def salt_first? Commenting the function as it is not being called anywhere
2. true
3. end
4. Methods that were just delegating their implementations to other functions in other classes

def admin?

 role.admin?

end def student? role.student? end was commented out and delegated using the code delegate :admin?, :student?, to: :role