CSC/ECE 517 Fall 2014/OSS E1464 vnn: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
 
(57 intermediate revisions by 2 users not shown)
Line 1: Line 1:
= E1464: Refactoring User model =
= E1464: Refactoring User model =
The requirements provided for the Open Source System project of Expertiza were as follows : <br>
The requirements provided for the Open Source System project of Expertiza were as follows : <br>
'''Classes''': <br>
== Classes ==
''user.rb'' (295 lines)<br>
The file refactored in this project is ''user.rb'' (295 lines) which is User Model class for expertiza project<br>


'''What they do''': <br>
==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.
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''': <br>  
== What is needed<ref>https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/</ref> ==
*Writing proper getters/setters if necessary, otherwise remove getters/setters
#Writing proper getters/setters if necessary, otherwise remove getters/setters
*Replacing inefficient boolean logic (`if true` is not conditional)
#Replacing inefficient boolean logic (`if true` is not conditional)
*Replacing old-style ActiveRecord queries
#Replacing old-style ActiveRecord queries
*Using {} and [] instead of Hash.new and Array.new
#Using {} and [] instead of Hash.new and Array.new
*Not instantiating more than 1 instance variable per action
#Not instantiating more than 1 instance variable per action


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


[http://expertiza.ncsu.edu/ 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 [https://github.com/expertiza/expertiza 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.
[http://expertiza.ncsu.edu/ Expertiza] is a project developed using [http://en.wikipedia.org/wiki/Ruby_on_Rails 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 [https://github.com/expertiza/expertiza 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.
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.
Line 22: Line 22:
= Project Resources =
= Project Resources =


[https://github.com/joshnikhil234/expertiza GitHub Project Repository]
#[https://github.com/joshnikhil234/expertiza GitHub Project Repository]
 
#[http://152.1.13.186:3000/ VCL IP Address]
[http://152.1.13.186:3000/ VCL IP Address]
#[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]
 
[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza Wiki]


= Background =
= Background =
Line 33: Line 31:
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.  
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.
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> =
= 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):
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):
#Inefficient boolean logic present at multiple lines in the code
#Inefficient boolean logic present at multiple lines in the code
Line 45: Line 42:
#Methods that were just delegating their implementations to other functions in other classes
#Methods that were just delegating their implementations to other functions in other classes


= Code Changes =
= 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:
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 =
== Inefficient boolean logic present at multiple lines in the code ==
<b>Before Changes</b><br>
:<b>Before Changes</b><br>
<pre>if elt==true
<pre>if elt==true
   yes
   yes
elsif elt ==false </pre>
elsif elt ==false </pre>


<b>After Changes</b><br>
:<b>After Changes</b><br>
<pre>if elt
<pre>if elt
   yes
   yes
elsif !elt</pre>
elsif !elt</pre>


== Not using [ ] & { } instead of Array.new & Hash.new ==
For reasons like code hygiene and efficiency, we did the following changes:<br>


 
:<b>Before Changes</b><br>
= Not using [ ] & { } instead of Array.new & Hash.new =
For reasons like code hygiene and efficiency, we did the following changes:<br>
<b>Before Changes</b><br>
<pre>tcsv = Array.new
<pre>tcsv = Array.new
</pre>
</pre>
<b>After Changes</b><br>
:<b>After Changes</b><br>
<pre>tcsv = []
<pre>tcsv = []</pre>
fields = Array.new
:<b>Before Changes</b><br>
was changed to
<pre>fields = Array.new</pre>
fields = []
:<b>After Changes</b><br>
</pre>
<pre>fields = []</pre>


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


<b>Before Changes</b><br>
:<b>Before Changes</b><br>
<pre>has_many :participants, :class_name => 'Participant', :foreign_key => 'user_id', :dependent => :destroy
<pre>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 :assignment_participants, :class_name => 'AssignmentParticipant', :foreign_key => 'user_id', :dependent => :destroy
Line 80: Line 76:
</pre>
</pre>


<b>After Changes</b><br>
:<b>After Changes</b><br>
<pre>has_many :participants, class_name: 'Participant', foreign_key: 'user_id', dependent: :destroy
<pre>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 :assignment_participants, class_name: 'AssignmentParticipant', foreign_key: 'user_id', dependent: :destroy
Line 86: Line 82:
</pre>
</pre>


<b>Before Changes</b><br>
:<b>Before Changes</b><br>
<pre>
<pre>
has_many :teams_users, :dependent => :destroy
has_many :teams_users, :dependent => :destroy
Line 92: Line 88:
</pre>
</pre>


<b>After Changes</b><br>
:<b>After Changes</b><br>
<pre>has_many :teams_users, dependent: :destroy
<pre>has_many :teams_users, dependent: :destroy
has_many :teams, through: :teams_users
has_many :teams, through: :teams_users
</pre>
</pre>


<b>Before Changes</b><br>
:<b>Before Changes</b><br>
<pre>
<pre>
has_many :children, class_name: 'User', :foreign_key => 'parent_id'
has_many :children, class_name: 'User', :foreign_key => 'parent_id'
</pre>
</pre>
<b>After Changes</b><br>
:<b>After Changes</b><br>
<pre>
<pre>
has_many :children, class_name: 'User', foreign_key: 'parent_id'
has_many :children, class_name: 'User', foreign_key: 'parent_id'
</pre>
</pre>


<b>Before Changes</b><br>
:<b>Before Changes</b><br>
<pre>
<pre>
validates_presence_of :email, :message => "can't be blank"
validates_presence_of :email, :message => "can't be blank"
Line 112: Line 108:
</pre>
</pre>


<b>After Changes</b><br>
:<b>After Changes</b><br>
<pre>
<pre>
validates_presence_of :email, message: "can't be blank"
validates_presence_of :email, message: "can't be blank"
Line 118: Line 114:
</pre>
</pre>


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


<b>Before Changes</b><br>
:<b>Before Changes</b><br>
<pre>all_users = User.all(conditions: ['name LIKE ?', "#{name}%"], limit: 20) # higher limit, since we're filtering
<pre>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')
@courses = Course.where(instructor_id: self.id, :order => 'name')
</pre>
</pre>
<b>After Changes</b><br>
:<b>After Changes</b><br>
<pre>
<pre>
@courses = Course.where(instructor_id: self.id, order: 'name')
@courses = Course.where(instructor_id: self.id, order: 'name')
</pre>
</pre>


<b>Before Changes</b><br>
:<b>Before Changes</b><br>
<pre>
<pre>
newuser = url_for :controller => 'users', :action => 'new'
newuser = url_for :controller => 'users', :action => 'new'
</pre>
</pre>
<b>After Changes</b><br>
:<b>After Changes</b><br>
<pre>
<pre>
newuser = url_for controller: 'users', action: 'new'
newuser = url_for controller: 'users', action: 'new'
</pre>
</pre>


= Not using in-built array functions such as one?, any?, zero?, first & last =
== Not using in-built array functions such as one?, any?, zero?, first & last ==
<b>Before Changes</b><br>
:<b>Before Changes</b><br>
<pre>
<pre>
if userList != nil && userList.length == 1
if userList != nil && userList.length == 1
</pre>
</pre>
<b>After Changes</b><br>
:<b>After Changes</b><br>
<pre>
<pre>
if userList != nil && userList.one?
if userList != nil && userList.one?
</pre>
</pre>


= Methods that were not being used =
== Methods that were not being used ==
<b>Before Changes</b><br>
:<b>Before Changes</b><br>
<pre>
<pre>
def salt_first?
def salt_first?
Line 158: Line 154:
end
end
</pre>
</pre>
<b>After Changes</b><br>
:<b>After Changes</b><br>
was commented out
 
:The function was commented out.
<pre>
<pre>
#def salt_first? //Commenting the function as it is not being called anywhere
#def salt_first? //Commenting the function as it is not being called anywhere
Line 166: Line 163:
</pre>
</pre>


= Methods that were just delegating their implementations to other functions in other classes =
== Methods that were just delegating their implementations to other functions in other classes ==
<b>Before Changes</b><br>
:<b>Before Changes</b><br>
<pre>
<pre>
def admin?
def admin?
Line 176: Line 173:
end
end
</pre>
</pre>
<b>After Changes</b><br>
:<b>After Changes</b><br>
was commented out and delegated using the code
 
:The code was commented out and delegated using the code
<pre>
<pre>
delegate :admin?, :student?, to: :role
delegate :admin?, :student?, to: :role
Line 187: Line 185:
1. Go to [http://152.1.13.186:3000/ VCL Expertiza]
1. Go to [http://152.1.13.186:3000/ VCL Expertiza]


[[File:main.png|30 px]]
[[File:main.png|frame|center]]


2. Enter Username as 'user2' OR 'user3' OR 'user4' and Password as 'password' where user2 is ADMIN and user3, user4 is STUDENT. Noew click on 'Login' button.  
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
3. You should now be able to view the landing page(given below) after successful login


[[File:success.png]]
[[File:success.png|frame|center]]


4. Now logout and try logging in with incorrect credentials. For Example: Username: 'user3' and Password: 'user3'
4. Now logout and try logging in with incorrect credentials. For Example: Username: 'user3' and Password: 'user3'
Line 199: Line 197:
5. You should be able to view the forgotten password page with message saying "Incorrect Name/Password"
5. You should be able to view the forgotten password page with message saying "Incorrect Name/Password"


[[File:Unsuccess.png]]
[[File:unsuccess.png|frame|center]]


6. Moreover, the forgotten password page can also be accessed directly, by clicking on "Forgotten your password?" link in the [http://152.1.13.186:3000/ VCL Expertiza] page
6. Moreover, the forgotten password page can also be accessed directly, by clicking on "Forgotten your password?" link in the [http://152.1.13.186:3000/ VCL Expertiza] page


[[File:forgotten.png]]
[[File:forgotten.png|frame|center]]


= Concise Report on Refactoring =
= 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.
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 =
[http://expertiza.ncsu.edu/ Expertiza]<br>
 
[http://en.wikipedia.org/wiki/Code_refactoring Code Refactoring]<br>
<references/>
[https://docs.google.com/document/d/1qQD7fcypFk77nq7Jx7ZNyCNpLyt1oXKaq5G-W7zkV3k/edit#heading=h.m3v4cs5yyc1u/ Global Rules For Refactoring]<br>
[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit#heading=h.qtd1lpemhaxe/ OSS Projects on Expertiza]<br>
[http://edgeguides.rubyonrails.org/4_0_release_notes.html/ Ruby on Rails 4.0]<br>

Latest revision as of 01:08, 5 November 2014

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