patternrubyMinor
Simplification of loop in Ruby
Viewed 0 times
looprubysimplification
Problem
I'm still pretty new to Ruby, and am really enjoying the language. I came across this method in my code base and realized that it could be cleaned up:
So I took the above and changed it to this:
One less level of nesting, and cleaner than the first method. But I look at this method and can't help but think there's still more to be done. Any ideas?
As some background
def friend_all_followers(friends)
client.followers.each do |follower|
if not friends.include?(follower.screen_name)
friend = Friend.new(:screen_name => follower.screen_name)
client.friendship_create(friend.screen_name, true)
friend.save
end
end
endSo I took the above and changed it to this:
def friend_all_followers(friends)
followers = client.followers.reject { |f| friends.include?(f) }
followers.each do |follower|
friend = Friend.new(:screen_name => follower.screen_name)
client.friendship_create(friend.screen_name, true)
friend.save
end
endOne less level of nesting, and cleaner than the first method. But I look at this method and can't help but think there's still more to be done. Any ideas?
As some background
Friend is an ActiveRecord class, client is a wrapper around a Twitter API, client.followers returns an array of follower objects - the objects I believe are just hashes, but I'm not 100% certain at the moment - and friends is a string array of screen_names.Solution
You can replace
With
I'd also suggest wrapping creation of friend in separate class method:
Then code becomes like this:
I'd probably hide the
update
Now whole code becomes like this:
Now you can do some readable operation on array. Something like this:
friend = Friend.new(:screen_name => follower.screen_name)
client.friendship_create(friend.screen_name, true)
friend.saveWith
friend = Friend.create(:screen_name => follower.screen_name)
client.friendship_create(friend.screen_name, true)I'd also suggest wrapping creation of friend in separate class method:
class Friend follower.screen_name
end
endThen code becomes like this:
friend = Friend.create_with_follower follower
client.friendship_create(friend.screen_name, true)I'd probably hide the
friendship_create method call somewhere too. Not sure what it does though. Is it separate API call?update
Now whole code becomes like this:
def friend_all_followers(friends)
followers = client.followers.reject { |f| friends.include?(f) }
followers.each do |follower|
friend = Friend.create_with_follower follower
client.friendship_create(friend.screen_name, true)
end
endNow you can do some readable operation on array. Something like this:
def friend_all_followers(friends)
(client.followers - friends).each do |follower|
friend = Friend.create_with_follower follower
client.friendship_create(friend.screen_name, true)
end
endCode Snippets
friend = Friend.new(:screen_name => follower.screen_name)
client.friendship_create(friend.screen_name, true)
friend.savefriend = Friend.create(:screen_name => follower.screen_name)
client.friendship_create(friend.screen_name, true)class Friend < ActiveRecord::Base
def create_with_follower(follower = nil)
return unless follower
create :screen_name => follower.screen_name
end
endfriend = Friend.create_with_follower follower
client.friendship_create(friend.screen_name, true)def friend_all_followers(friends)
followers = client.followers.reject { |f| friends.include?(f) }
followers.each do |follower|
friend = Friend.create_with_follower follower
client.friendship_create(friend.screen_name, true)
end
endContext
StackExchange Code Review Q#2467, answer score: 7
Revisions (0)
No revisions yet.