CSC/ECE 517 Fall 2017/E1746 Refactor user.rb
Jump to navigation
Jump to search
E1746 [Test First Development] Refactor user.rb
This page provides a description of the Expertiza based OSS project.
About Expertiza
Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.
Test Plan
The following tasks were accomplished in this project:
- Complete the pending tests in user_spec.rb
- There are many pending tests needed to be finished. And for the methods which are needed to be refactored or renamed, this is also can been seen as writing failing tests first.
- Refactor get_user_list method.
- The get_user_list methods contain many conditions and each of them has a lot of code which makes the method very long. The factor is to transfer the statements to the corresponding subclasses ,and writing method calls instead.
- Refactor self.search_users method
- The method has many repetitive codes which are used in the conditionals. We refactor it by writing a single method call and let the method call to choose the conditional.
- Rename methods and change all other places it is used.
- Use find_by instead of dynamic method.
- The find_by_name or find_by_email method which are used in the user.rb is like old style code. And this kind of code can be replaced with new dynamic ones like, find_by.
Missions and Solutions
Mission 1:Complete the pending tests in user_spec.rb
- There are many pending tests in the file 'user.rb'. What we have done is finishing these pending tests and makes them pass.The user_spec.rb file on github is below.
https://github.com/AnranZhou/expertiza/blob/master/spec/models/user_spec.rb
Mission 2:Refactor get_user_list method
- The task is to write failing test first and ,move if conditions to corresponding subclasses (eg. ta.rb, instructor.rb) with same method name,then replace the conditional with the relevant method calls.
- First writing the failing test first according to the requirements of the refactor.
#describe '#get_user_list' do
context 'when current user is super admin' do
it 'fetches all users' do
allow(user).to receive_message_chain("role.super_admin?"){ true }
allow(user).to receive_message_chain("role.instructor?"){ false }
allow(user).to receive_message_chain("role.ta?"){false}
allow(SuperAdministrator).to receive(:get_user_list).and_return([user1,user2])
expect(user.get_user_list).to eq ([user1,user2])
end
end
context 'when current user is an instructor' do
it 'fetches all users in his/her course/assignment' do
allow(user).to receive_message_chain("role.super_admin?"){ false }
allow(user).to receive_message_chain("role.instructor?"){ true }
allow(user).to receive_message_chain("role.ta?"){false}
allow(Instructor).to receive(:get_user_list).and_return([user1,user2])
expect(user.get_user_list).to eq ([user1,user2])
end
end
context 'when current user is a TA' do
it 'fetches all users in his/her courses'do
allow(user).to receive_message_chain("role.super_admin?"){ false }
allow(user).to receive_message_chain("role.ta?"){ true }
allow(user).to receive_message_chain("role.instructor?"){ false }
allow(Ta).to receive(:get_user_list).and_return([user1,user2])
expect(user.get_user_list).to eq ([user1,user2])
end
end
end
- The next step is to move if conditionals to its relatively subclasses. In this case,the superadministrator.rb, instructor.rb and ta.rb.
#class SuperAdministrator < User
def self.get_user_list
user_list=[]
User.all.find_each do |user|
user_list << user
end
end
end
#def self.get_user_list
user_list=[]
participants = []
Course.where(instructor_id: self.id).find_each do |course|
participants << course.get_participants
end
Assignment.where(instructor_id: self.id).find_each do |assignment|
participants << assignment.participants
end
participants.each do |p_s|
next if p_s.empty?
p_s.each do |p|
user_list << p.user if self.role.hasAllPrivilegesOf(p.user.role)
end
end
end
#def self.get_user_list
get_user_list=[]
courses.each do |course_id|
course = Course.find(course_id)
participants << course.get_participants
end
participants.each do |p_s|
next if p_s.empty?
p_s.each do |p|
user_list << p.user if self.role.hasAllPrivilegesOf(p.user.role)
end
end
end
- Then replace the conditional with the relevant method calls.
#def get_user_list
user_list = []
# If the user is a super admin, fetch all users
if self.role.super_admin?
return SuperAdministrator.get_user_list
end
# If the user is an instructor, fetch all users in his course/assignment
if self.role.instructor?
return Instructor.get_user_list
end
# If the user is a TA, fetch all users in his courses
if self.role.ta?
return Ta.get_user_list
end
# Add the children to the list
unless self.role.super_admin?
User.all.find_each do |u|
if is_recursively_parent_of(u)
user_list << u unless user_list.include?(u)
end
end
end
user_list.uniq
end
- So the refactor of get_user_list method has been done.
Mission 3:Refactor self.search_users method.
- This task is required to write the failing test first and extract duplicated code to a new method.
- First, write the failing test.
#descirbe '.search_users' do
let(:role) { Role.new }
before(:each) do
allow(User).to receive_message_chain(:order,:where).and_return(user)
end
it 'when the search_by is 1' do
search_by = "1"
user_id = double
letter = 'name'
search_filter = '%' + letter + '%'
expect(User).to receive_message_chain(:order,:where).with("(role_id in (?) or id = ?) and name like ?", role.get_available_roles, user_id, search_filter)
expect(User.search_users(role,user_id,letter,search_by)).to eq user
end
it 'when the search_by is 2' do
search_by = "2"
user_id = double
letter = 'fullname'
search_filter = '%' + letter + '%'
expect(User).to receive_message_chain(:order,:where).with("(role_id in (?) or id = ?) and fullname like ?", role.get_available_roles, user_id, search_filter)
expect(User.search_users(role,user_id,letter,search_by)).to eq user
end
it 'when the search_by is 3' do
search_by = "3"
user_id = double
letter = 'email'
search_filter = '%' + letter + '%'
expect(User).to receive_message_chain(:order,:where).with("(role_id in (?) or id = ?) and email like ?", role.get_available_roles, user_id, search_filter)
expect(User.search_users(role,user_id,letter,search_by)).to eq user
end
it 'when the search_by is default value' do
search_by = nil
user_id = double
letter = ''
search_filter = letter + '%'
expect(User).to receive_message_chain(:order,:where).with("(role_id in (?) or id = ?) and name like ?", role.get_available_roles, user_id, search_filter)
expect(User.search_users(role,user_id,letter,search_by)).to eq user
end
end
- Then,extract duplicated code to a new method.Since this is kind of changing original method.So below is the changing on github.
# def self.search_users(role, user_id, letter, search_by)
- if search_by == '1' # search by user name
+ key_word = {'1'=>'name','2'=>'fullname','3'=>'email'}
+ if key_word.include? search_by
search_filter = '%' + letter + '%'
- users = User.order('name').where("(role_id in (?) or id = ?) and name like ?", role.get_available_roles, user_id, search_filter)
- elsif search_by == '2' # search by full name
- search_filter = '%' + letter + '%'
- users = User.order('name').where("(role_id in (?) or id = ?) and fullname like ?", role.get_available_roles, user_id, search_filter)
- elsif search_by == '3' # search by email
- search_filter = '%' + letter + '%'
- users = User.order('name').where("(role_id in (?) or id = ?) and email like ?", role.get_available_roles, user_id, search_filter)
+ users = User.order('name').where("(role_id in (?) or id = ?) and #{key_word[search_by]} like ?", role.get_available_roles, user_id, search_filter)
else # default used when clicking on letters
search_filter = letter + '%'
users = User.order('name').where("(role_id in (?) or id = ?) and name like ?", role.get_available_roles, user_id, search_filter)
Mission 4:Rename method and change all other places it is used
- First, is to write failing test of the renamed method which has been finished in mission one.So just skip to the second part to rename them.
- is_recursively_parent_of → recursively_parent_of?
- expect(user1.is_recursively_parent_of(user)).to eq false + expect(user1.recursively_parent_of(user)).to eq false
- is_creator_of? → creator_of?
- describe '#is_creator_of?' do
+ describe '#creator_of?' do
it 'returns true of current user (user) is the creator of target user (user1)' do
allow(user1).to receive(:creator).and_return(user)
- expect(user.is_creator_of?(user1)).to be true
+ expect(user.creator_of?(user1)).to be true
end
it 'returns false of current user (user) is not the creator of target user (user1)' do
allow(user1).to receive(:creator).and_return(user2)
- expect(user.is_creator_of?(user1)).to be false
- expect(user2.is_creator_of?(user1)).to be true
+ expect(user.creator_of?(user1)).to be false
+ expect(user2.creator_of?(user1)).to be true
end
end
@@ -382,16 +382,16 @@
end
end
- is_teaching_assistant_for? → teaching_assistant_for?
- describe '#is_teaching_assistant_for?' do
+ describe '#teaching_assistant_for?' do
it 'returns false if current user is not a TA' do
allow(user1).to receive_message_chain("role.ta?"){ false }
- expect(user1.is_teaching_assistant_for?(user)).to be_falsey
+ expect(user1.teaching_assistant_for?(user)).to be_falsey
end
it 'returns false if current user is a TA, but target user is not a student'do
allow(user1).to receive_message_chain("role.ta?"){true }
allow(user).to receive_message_chain("role.name").and_return('teacher')
- expect(user1.is_teaching_assistant_for?(user)).to be_falsey
+ expect(user1.teaching_assistant_for?(user)).to be_falsey
end
it 'returns true if current user is a TA of target user'do
@@ -401,23 +401,23 @@
c1=Course.new
allow(user1).to receive_message_chain(:courses_assisted_with,:any?).and_yield(c1)
allow_any_instance_of(Course).to receive_message_chain(:assignments,:map,:flatten,:map,:include?,:user).and_return(true)
- expect(user1.is_teaching_assistant_for?(user)).to be true
+ expect(user1.teaching_assistant_for?(user)).to be true
end
end
- is_teaching_assistant? → teaching_assistant?
- describe '#is_teaching_assistant?' do
+ describe '#teaching_assistant?' do
it 'returns true if current user is a TA' do
allow(user).to receive_message_chain("role.ta?"){ true }
- expect(user.is_teaching_assistant?).to be true
+ expect(user.teaching_assistant?).to be true
end
it 'returns false if current user is not a TA' do
allow(user).to receive_message_chain("role.ta?"){ false }
- expect(user.is_teaching_assistant?).to be_falsey
+ expect(user.teaching_assistant?).to be_falsey
end
end
- After writing the test, it is going to rename the method in the user.rb. Below is the changed part of the code.
- return true if self.is_teaching_assistant_for?(user) - return true if self.is_recursively_parent_of(user) + return true if self.teaching_assistant_for?(user) + return true if self.recursively_parent_of(user) - def is_recursively_parent_of(user) + def recursively_parent_of(user) - self.is_recursively_parent_of(p) + self.recursively_parent_of(p) - if is_recursively_parent_of(u) + if recursively_parent_of(u) - def is_creator_of?(user) + def creator_of?(user) - def is_teaching_assistant_for?(student) - return false unless is_teaching_assistant? + def teaching_assistant_for?(student) + return false unless teaching_assistant? - def is_teaching_assistant? + def teaching_assistant?
Mission 5:Use find_by instead of dynamic method
：：There are three lines using find_by_name and find_by_email which are now be instead of find_by[params].The task is to replace the old one to refactor the method.
- First,writing the failing tests,here is the original test code including find_by method.
allow(User).to receive(:find_by_name).and_return(user)
And below is the new failing test
allow(User).to receive(:find_by).and_return(user)
And so as the other codes contain find_by method
- allow(User).to receive(:find_by).with(email).and_return(user) + allow(User).to receive(:find_by).and_return(user) - allow(User).to receive(:find_by_email).and_return(nil) + allow(User).to receive(:find_by).and_return(nil) - allow(User).to receive(:find_by_name).and_return(user) + allow(User).to receive(:find_by).and_return(user)
And then change the find_by_name and find_by_name in the user.rb file in to dynamic one.
- user = User.find_by_name(row[0]) + user = User.find_by name: row[0] - user = User.find_by_email(login) + user = User.find_by email: login - User.find_by_name(params[:user][:name]) + User.find_by name: params[:user][:name]
References
- Expertiza on GitHub
- GitHub Project Repository Fork
- The live Expertiza website
- Demo link
- Expertiza project documentation wiki
- Rspec Documentation
- Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin