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

The file refactored in this project is user.rb (295 lines) which is User Model class for expertiza project

What they do

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

What is needed<ref>https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/</ref>

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

Introduction to Expertiza<ref>http://expertiza.ncsu.edu/</ref>

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

1. GitHub Project Repository
2. VCL IP Address
3. 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 handles 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<ref>https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/</ref>

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<ref>http://edgeguides.rubyonrails.org/4_0_release_notes.html/</ref>

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, we did the following changes:

Before Changes

tcsv = Array.new

After Changes

tcsv = []

Before Changes

fields = Array.new

After Changes

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

The function 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

The code was commented out and delegated using the code

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

Steps to verify changes

We have refactored the code for user.rb file (User Model File) for Expertiza project without changing the functionality of the expertiza project. As such there would be no changes in the functionality of the project, but inorder to verfiy the changes made please follow the below mentioned steps:

1. Go to VCL Expertiza

2. Enter Username as 'user2' OR 'user3' OR 'user4' and Password as 'password' where user2 is ADMIN and user3, user4 are STUDENT. Now click on 'Login' button.

3. You should now be able to view the landing page(given below) after successful login

4. Now logout and try logging in with incorrect credentials. For Example: Username: 'user3' and Password: 'user3'

5. You should be able to view the forgotten password page with message saying "Incorrect Name/Password"

6. Moreover, the forgotten password page can also be accessed directly, by clicking on "Forgotten your password?" link in the VCL Expertiza page

Concise Report on Refactoring<ref>http://en.wikipedia.org/wiki/Code_refactoring</ref>

The project had redundant code, multiple functionalities in a method and extra conditional statements in User Model file. We analyzed each occurrence of these issues, thus creating a list which needed to be refactored. Thus we refactored the User Model file according to global rules for Model class file and Rails 4.0 standard. It's very difficult to explain the sequential and detailed procedure, but it has been explained in the sections above within the capacity of the Wiki.

References

<references/>