CSC/ECE 517 Fall 2014/oss E1465 oak: Difference between revisions
(41 intermediate revisions by 3 users not shown) | |||
Line 4: | Line 4: | ||
==Introduction== | ==Introduction== | ||
Expertiza<ref name="expertiza>''Expertiza'' http://wikis.lib.ncsu.edu/index.php/Expertiza</ref> is a web application available to both students and professors. The Expertiza project is a system to create reusable learning objects through peer review. Expertiza supports team projects | Expertiza<ref name="expertiza>''Expertiza'' http://wikis.lib.ncsu.edu/index.php/Expertiza</ref> is a web application available to both students and professors. The Expertiza project is a system to create reusable learning objects through peer review. Expertiza supports team projects and the submission of almost any document type, including URLs and wiki pages. Students can keep a track of their assignments, teammates and can conduct peer reviews on diverse topics and projects. It is an open source project developed on Ruby on Rails platform. More information on Expertiza can be found [https://github.com/expertiza/expertiza here]. The source code can be forked and cloned for making modifications. | ||
As a part of the coursework of Object Oriented Design and Development, we were expected to refactor the funtionality of some modules of Expertiza. This wiki provides an insight into our contributions to the Open Source Software project Expertiza | As a part of the coursework of Object Oriented Design and Development, we were expected to refactor the funtionality of some modules of Expertiza. This wiki provides an insight into our contributions to the Open Source Software project 'Expertiza' by Refactoring the Users Controller. | ||
==Project Description== | ==Project Description== | ||
Line 15: | Line 15: | ||
<b>What has to be changed : </b> | <b>What has to be changed : </b> | ||
* Modify methods to conform to RESTful style | * Modify methods to conform to RESTful style | ||
* Reducing the number of instance variables per action to one | * Reducing the number of instance variables per action to one | ||
* Code | * Code cleanup by using string interpolation instead of concatenation, replacing '==' with eql? and :key =>'value' with key: 'value', modifying declarations of Arrays and Hashes and removing commented out code | ||
* Use of routing helpers instead of hardcoded URLs | * Use of routing helpers instead of hardcoded URLs | ||
* Replace find_by with where to follow Rails 4.0 conventions | * Replace find_by with where to follow Rails 4.0 conventions | ||
<br> | |||
<br> | |||
Follow the steps below to access the view for UsersController. | |||
<br> | |||
1. Access the homepage of application and login as either super admin, admin, Instructor or Teaching Assistant. | |||
[[File:step1.jpg]] | |||
<br> | |||
<br> | |||
<br> | |||
2. Hover over the "Manage" menu option and click on "Users". | |||
[[File:step2.jpg]] | |||
<br> | |||
<br> | |||
<br> | |||
3. The view rendered is the index view for UsersControllers. | |||
[[File:step3.jpg]] | |||
==Modification to Existing Code== | ==Modification to Existing Code== | ||
=== | ===RESTful style implementation=== | ||
* | * Modifications to users_controller.rb : The purpose of the index method in Users Controller is to list all registered users. The list method was called from the index method to list all users. This implementation did not conform to RESTful guidelines. Hence, we refactored the list method in users controller to index. Further, we refactored all the references of list method in UserControllers, Users views, Users tests and routes.rb to the index method. | ||
Before Refactoring : | Before Refactoring : | ||
< | <pre> | ||
def index | def index | ||
if (current_user_role? == "Student") | if (current_user_role? == "Student") | ||
redirect_to(:action => AuthHelper::get_home_action(session[:user]), :controller => AuthHelper::get_home_controller(session[:user])) | redirect_to(:action => AuthHelper::get_home_action(session[:user]), :controller => AuthHelper::get_home_controller(session[:user])) | ||
Line 72: | Line 88: | ||
@letters = ('A'..'Z').to_a | @letters = ('A'..'Z').to_a | ||
end | end | ||
</ | </pre> | ||
After Refactoring : | After Refactoring : | ||
< | <pre> | ||
#for displaying the list of users | #for displaying the list of users | ||
def index | def index | ||
if (current_user_role | if (current_user_role? == "Student") | ||
redirect_to(action | redirect_to(:action => AuthHelper::get_home_action(session[:user]), :controller => AuthHelper::get_home_controller(session[:user])) | ||
else | else | ||
user = | #list method's implementation | ||
end | |||
end | |||
</pre> | |||
===Instance Variable Reductions=== | |||
It is a bad practice to have more than one instance variables in a controller's method as it indicates increased coupling. Coupling must be as less as possible and the view should have direct access to as few instance variables as possible. Helper methods should be defined in controllers through which the view can access variables of the controller class. The code has been refactored to make use of helper methods for the instance variables in index action. | |||
<b>Before Refactoring</b> | |||
users_controller.rb | |||
<pre> | |||
def get_role | |||
if @user && @user.role_id | |||
@role = Role.find(@user.role_id) | |||
elsif @user | |||
@role = Role.new(:id => nil, :name => '(none)') | |||
end | |||
end | |||
</pre> | |||
show.html.erb | |||
<pre> | |||
<tr><th>Role:</th> | |||
<td><%= link_to @role.name, :controller => 'roles', | |||
:action => 'show', :id => @role.id %></td> | |||
</tr> | |||
</pre> | |||
<b>After Refactoring</b> | |||
users_controller.rb | |||
<pre> | |||
def get_role | |||
if @user && @user.role_id | |||
role = Role.find(@user.role_id) | |||
elsif @user | |||
role = Role.new(id: nil, name: '(none)') | |||
end | |||
return role | |||
end | |||
</pre> | |||
show.html.erb | |||
<pre> | |||
<tr><th>Role:</th> | |||
<td><%= link_to get_role.name, :controller => 'roles', | |||
:action => 'show', :id => get_role.id %></td> | |||
</tr> | |||
</table> | |||
</pre> | |||
Similar approach for reduction of instance variables has been implemented in other methods like index, show_selection, etc. | |||
===Code Cleanup=== | |||
Code cleanup in the controller so that the code conforms closely to Rails conventions and further enhances the readability of the code. | |||
* Replaced String concats with #{} in paginate_list | |||
Before Refactoring : | |||
<pre> | |||
#paginate_list | |||
search_filter = letter + '%' | |||
search_filter = '%' + letter + '%' | |||
</pre> | |||
After Refactoring : | |||
<pre> | |||
#paginate_list | |||
search_filter = "#{letter}%" | |||
search_filter = "%#{letter}%" | |||
</pre> | |||
* Replaced '==' with eql? | |||
Before Refactoring : | |||
<pre> | |||
#index | |||
if (current_user_role? == "Student") | |||
... | |||
letter == nil | |||
#show_selection | |||
@role.parent_id == nil || @role.parent_id < (session[:user]).role_id || @user.id == (session[:user]).id | |||
#show | |||
if (params[:id].nil?) || ((current_user_role? == "Student") && (session[:user].id != params[:id].to_i)) | |||
#create | |||
if @user.role.name == "Instructor" or @user.role.name == "Administrator" | |||
#keys | |||
if (params[:id].nil?) || ((current_user_role? == "Student") && (session[:user].id != params[:id].to_i)) | |||
#paginate_list | |||
if @search_by == '1' | |||
</pre> | |||
After Refactoring : | |||
<pre> | |||
#index | |||
if (current_user_role.eql? "Student") | |||
... | |||
if letter.eql? nil | |||
#show_selection | |||
if role.parent_id.eql? nil || role.parent_id < (session[:user]).role_id || @user.id.eql?(session[:user]).id | |||
#show | |||
if (params[:id].nil?) || ((current_user_role.eql? "Student") && (session[:user].id != params[:id].to_i)) | |||
#create | |||
if @user.role.name.eql? "Instructor" or @user.role.name.eql? "Administrator" | |||
#keys | |||
if (params[:id].nil?) || ((current_user_role.eql? "Student") && (session[:user].id != params[:id].to_i)) | |||
#paginate_list | |||
if @search_by.eql? '1' #search by user name | |||
</pre> | |||
* Replaced :key =>'value' with key: 'value' | |||
Before Refactoring : | |||
<pre> | |||
#index | |||
redirect_to(:action => AuthHelper::get_home_action(session[:user]), :controller => AuthHelper::get_home_controller(session[:user])) | |||
#show_selection | |||
render :action => 'show' | |||
#create | |||
AssignmentQuestionnaire.create(:user_id => @user.id) | |||
</pre> | |||
After Refactoring : | |||
<pre> | |||
#index | |||
redirect_to(action: AuthHelper::get_home_action(session[:user]), controller: AuthHelper::get_home_controller(session[:user])) | |||
#show_selection | |||
render action: 'show' | |||
#create | |||
AssignmentQuestionnaire.create(user_id: @user.id) | |||
</pre> | |||
*Modified declarations of Arrays and Hashes | |||
Before Refactoring : | |||
<pre> | |||
#index | |||
@letters = Array.new | |||
#paginate_list | |||
paginate_options = {"1" => 25, "2" => 50, "3" => 100} | |||
</pre> | |||
After Refactoring : | |||
<pre> | |||
#index | |||
@letters =[] | |||
#paginate_list | |||
paginate_options = Hash["1" ,25 , "2", 50, "3", 100] | |||
</pre> | |||
*Removed unused methods like self.participants_in and commented out code. | |||
===Use of Routing Helpers=== | |||
Routing helpers are a simpler alternative to the otherwise complex hard coded URLs which reduce the readability of the code.Routing helpers allow us to declare possible common routes for a given controller. Routing helpers have been implemented since they maintain consistency even if changes are made to the routing paths. | |||
Before Refactoring: | |||
<br> | |||
if | <pre> | ||
def show_selection | |||
@user = User.find_by_name(params[:user][:name]) | |||
if @user != nil | |||
get_role | |||
if @role.parent_id == nil || @role.parent_id < (session[:user]).role_id || @user.id == (session[:user]).id | |||
render :action => 'show' | |||
else | |||
flash[:note] = 'The specified user is not available for editing.' | |||
redirect_to :action => 'list' | |||
end | |||
else | |||
flash[:note] = params[:user][:name]+' does not exist.' | |||
redirect_to :action => 'list' | |||
end | |||
end | |||
</pre> | |||
<br> | |||
After Refactoring | |||
<br> | |||
<pre> | |||
def show_selection | |||
@user = User.where(name: params[:user][:name]).take | |||
if @user != nil | |||
role=get_role | |||
if role.parent_id.eql? nil || role.parent_id < (session[:user]).role_id || @user.id.eql?(session[:user]).id | |||
render action: 'show' | |||
else | |||
flash[:note] = 'The specified user is not available for editing.' | |||
redirect_to users_path #Replaced URL with routing helper | |||
end | |||
else | |||
flash[:note] = params[:user][:name]+' does not exist.' | |||
redirect_to users_path #Replaced URL with routing helper | |||
end | |||
end | end | ||
</pre> | |||
@ | <br> | ||
===Replace find_by_.. with where in querying=== | |||
Rails 4 conventions dictate the use of 'where()' over the use of 'find_by_...' methods while querying ActiveRecords. The code has been refactored to replace the usages of find_by.. with where(). | |||
Before Refactoring | |||
<pre> | |||
def show_selection | |||
@user = User.find_by_name(params[:user][:name]) | |||
if @user != nil | |||
</pre> | |||
After Refactoring | |||
<pre> | |||
def show_selection | |||
</ | @user = User.where(name: params[:user][:name]).take | ||
if @user != nil | |||
</pre> | |||
== | ==Comparison of Original and Refactored Code== | ||
Major refactoring revolved around moving lines of codes between methods, modifying array and hash declarations, using helper methods for avoiding multiple instance variables in controller methods and implementing Ruby Conventions.<br> | |||
*Lines of Code | |||
Original : 248<br> | |||
Post Refactoring : 224 | |||
*Code Complexity (Compared on Code Climate) | |||
Original UsersController<ref name="originaluserscontroller>''Original UsersController'' https://codeclimate.com/github/expertiza/expertiza/UsersController</ref> | |||
<br/> | |||
[[File:Codeclimate expertiza.jpg]] | |||
<br/> | |||
Refactored UsersController<ref name="refactoreduserscontroller>''Refactored UsersController'' https://codeclimate.com/repos/5450f2eae30ba012a6011c01/UsersController</ref> | |||
<br/> | |||
[[File:Codeclimate users.jpg]] | |||
==Further Readings== | |||
1.[https://github.com/ameyp1992/expertiza.git Git Repository] | |||
2.[http://wiki.expertiza.ncsu.edu/index.php/Development:Setup:Linux:Debian Expertiza Setup] | |||
3.[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit?pli=1#heading=h.2y12n9sli9rd Project requirements] | |||
==References== | ==References== | ||
<references/><br> | <references/><br> |
Latest revision as of 03:02, 30 October 2014
Expertiza - Refactoring UsersController
Introduction
Expertiza<ref name="expertiza>Expertiza http://wikis.lib.ncsu.edu/index.php/Expertiza</ref> is a web application available to both students and professors. The Expertiza project is a system to create reusable learning objects through peer review. Expertiza supports team projects and the submission of almost any document type, including URLs and wiki pages. Students can keep a track of their assignments, teammates and can conduct peer reviews on diverse topics and projects. It is an open source project developed on Ruby on Rails platform. More information on Expertiza can be found here. The source code can be forked and cloned for making modifications.
As a part of the coursework of Object Oriented Design and Development, we were expected to refactor the funtionality of some modules of Expertiza. This wiki provides an insight into our contributions to the Open Source Software project 'Expertiza' by Refactoring the Users Controller.
Project Description
The Users Controller deals with managing activities peripheral to the User registered with Expertiza. Users are mapped to different roles like super-admin, admin, instructor, teaching assistant and student with each role having different access permissions. The Manage Users module can be accessed by roles other than the student. It provides users with the functionality to search other users based on keywords like username, email, etc. New user can be created and existing user information and role can be updated.
Classes : users_controller.rb
What it does : Manage Users i.e.search and list users, create new user, edit/update existing users, delete users.
What has to be changed :
- Modify methods to conform to RESTful style
- Reducing the number of instance variables per action to one
- Code cleanup by using string interpolation instead of concatenation, replacing '==' with eql? and :key =>'value' with key: 'value', modifying declarations of Arrays and Hashes and removing commented out code
- Use of routing helpers instead of hardcoded URLs
- Replace find_by with where to follow Rails 4.0 conventions
Follow the steps below to access the view for UsersController.
1. Access the homepage of application and login as either super admin, admin, Instructor or Teaching Assistant.
2. Hover over the "Manage" menu option and click on "Users".
3. The view rendered is the index view for UsersControllers.
Modification to Existing Code
RESTful style implementation
- Modifications to users_controller.rb : The purpose of the index method in Users Controller is to list all registered users. The list method was called from the index method to list all users. This implementation did not conform to RESTful guidelines. Hence, we refactored the list method in users controller to index. Further, we refactored all the references of list method in UserControllers, Users views, Users tests and routes.rb to the index method.
Before Refactoring :
def index if (current_user_role? == "Student") redirect_to(:action => AuthHelper::get_home_action(session[:user]), :controller => AuthHelper::get_home_controller(session[:user])) else list render :action => 'list' end end #for displaying the list of users def list user = session[:user] role = Role.find(user.role_id) all_users = User.order('name').where( ['role_id in (?) or id = ?', role.get_available_roles, user.id]) letter = params[:letter] session[:letter] = letter if letter == nil letter = all_users.first.name[0,1].downcase end logger.info "#{letter}" @letters = Array.new @per_page = 1 # Check if the "Show" button for pagination is clicked # If yes, set @per_page to the value of the selection dropdown # Else, if the request is from one of the letter links on the top # set @per_page to 1 (25 names per page). # Else, set @per_page to the :num_users param passed in from # the will_paginate method from the 'pagination' partial. if params[:paginate_show] @per_page = params[:num_users] elsif params[:from_letter] @per_page = 1 else @per_page = params[:num_users] end # Get the users list to show on current page @users = paginate_list(role, user.id, letter) @letters = ('A'..'Z').to_a end
After Refactoring :
#for displaying the list of users def index if (current_user_role? == "Student") redirect_to(:action => AuthHelper::get_home_action(session[:user]), :controller => AuthHelper::get_home_controller(session[:user])) else #list method's implementation end end
Instance Variable Reductions
It is a bad practice to have more than one instance variables in a controller's method as it indicates increased coupling. Coupling must be as less as possible and the view should have direct access to as few instance variables as possible. Helper methods should be defined in controllers through which the view can access variables of the controller class. The code has been refactored to make use of helper methods for the instance variables in index action.
Before Refactoring
users_controller.rb
def get_role if @user && @user.role_id @role = Role.find(@user.role_id) elsif @user @role = Role.new(:id => nil, :name => '(none)') end end
show.html.erb
<tr><th>Role:</th> <td><%= link_to @role.name, :controller => 'roles', :action => 'show', :id => @role.id %></td> </tr>
After Refactoring
users_controller.rb
def get_role if @user && @user.role_id role = Role.find(@user.role_id) elsif @user role = Role.new(id: nil, name: '(none)') end return role end
show.html.erb
<tr><th>Role:</th> <td><%= link_to get_role.name, :controller => 'roles', :action => 'show', :id => get_role.id %></td> </tr> </table>
Similar approach for reduction of instance variables has been implemented in other methods like index, show_selection, etc.
Code Cleanup
Code cleanup in the controller so that the code conforms closely to Rails conventions and further enhances the readability of the code.
- Replaced String concats with #{} in paginate_list
Before Refactoring :
#paginate_list search_filter = letter + '%' search_filter = '%' + letter + '%'
After Refactoring :
#paginate_list search_filter = "#{letter}%" search_filter = "%#{letter}%"
- Replaced '==' with eql?
Before Refactoring :
#index if (current_user_role? == "Student") ... letter == nil #show_selection @role.parent_id == nil || @role.parent_id < (session[:user]).role_id || @user.id == (session[:user]).id #show if (params[:id].nil?) || ((current_user_role? == "Student") && (session[:user].id != params[:id].to_i)) #create if @user.role.name == "Instructor" or @user.role.name == "Administrator" #keys if (params[:id].nil?) || ((current_user_role? == "Student") && (session[:user].id != params[:id].to_i)) #paginate_list if @search_by == '1'
After Refactoring :
#index if (current_user_role.eql? "Student") ... if letter.eql? nil #show_selection if role.parent_id.eql? nil || role.parent_id < (session[:user]).role_id || @user.id.eql?(session[:user]).id #show if (params[:id].nil?) || ((current_user_role.eql? "Student") && (session[:user].id != params[:id].to_i)) #create if @user.role.name.eql? "Instructor" or @user.role.name.eql? "Administrator" #keys if (params[:id].nil?) || ((current_user_role.eql? "Student") && (session[:user].id != params[:id].to_i)) #paginate_list if @search_by.eql? '1' #search by user name
- Replaced :key =>'value' with key: 'value'
Before Refactoring :
#index redirect_to(:action => AuthHelper::get_home_action(session[:user]), :controller => AuthHelper::get_home_controller(session[:user])) #show_selection render :action => 'show' #create AssignmentQuestionnaire.create(:user_id => @user.id)
After Refactoring :
#index redirect_to(action: AuthHelper::get_home_action(session[:user]), controller: AuthHelper::get_home_controller(session[:user])) #show_selection render action: 'show' #create AssignmentQuestionnaire.create(user_id: @user.id)
- Modified declarations of Arrays and Hashes
Before Refactoring :
#index @letters = Array.new #paginate_list paginate_options = {"1" => 25, "2" => 50, "3" => 100}
After Refactoring :
#index @letters =[] #paginate_list paginate_options = Hash["1" ,25 , "2", 50, "3", 100]
- Removed unused methods like self.participants_in and commented out code.
Use of Routing Helpers
Routing helpers are a simpler alternative to the otherwise complex hard coded URLs which reduce the readability of the code.Routing helpers allow us to declare possible common routes for a given controller. Routing helpers have been implemented since they maintain consistency even if changes are made to the routing paths.
Before Refactoring:
def show_selection @user = User.find_by_name(params[:user][:name]) if @user != nil get_role if @role.parent_id == nil || @role.parent_id < (session[:user]).role_id || @user.id == (session[:user]).id render :action => 'show' else flash[:note] = 'The specified user is not available for editing.' redirect_to :action => 'list' end else flash[:note] = params[:user][:name]+' does not exist.' redirect_to :action => 'list' end end
After Refactoring
def show_selection @user = User.where(name: params[:user][:name]).take if @user != nil role=get_role if role.parent_id.eql? nil || role.parent_id < (session[:user]).role_id || @user.id.eql?(session[:user]).id render action: 'show' else flash[:note] = 'The specified user is not available for editing.' redirect_to users_path #Replaced URL with routing helper end else flash[:note] = params[:user][:name]+' does not exist.' redirect_to users_path #Replaced URL with routing helper end end
Replace find_by_.. with where in querying
Rails 4 conventions dictate the use of 'where()' over the use of 'find_by_...' methods while querying ActiveRecords. The code has been refactored to replace the usages of find_by.. with where().
Before Refactoring
def show_selection @user = User.find_by_name(params[:user][:name]) if @user != nil
After Refactoring
def show_selection @user = User.where(name: params[:user][:name]).take if @user != nil
Comparison of Original and Refactored Code
Major refactoring revolved around moving lines of codes between methods, modifying array and hash declarations, using helper methods for avoiding multiple instance variables in controller methods and implementing Ruby Conventions.
- Lines of Code
Original : 248
Post Refactoring : 224
- Code Complexity (Compared on Code Climate)
Original UsersController<ref name="originaluserscontroller>Original UsersController https://codeclimate.com/github/expertiza/expertiza/UsersController</ref>
Refactored UsersController<ref name="refactoreduserscontroller>Refactored UsersController https://codeclimate.com/repos/5450f2eae30ba012a6011c01/UsersController</ref>
Further Readings
References
<references/>