Simplification of loop in Ruby

Question

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:

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 end 

So 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 end 

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

Answer 1

You can replace

friend = Friend.new(:screen_name => follower.screen_name) client.friendship_create(friend.screen_name, true) friend.save 

With

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 < ActiveRecord::Base def create_with_follower(follower = nil) return unless follower create :screen_name => follower.screen_name end end 

Then 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 end 

Now 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 end 

Answer 2

Letting this percolate, I realized that I'm doing two things in friend_all_followers. The first is filtering new followers based on old followers (calling the variable friends obscures that, so I'll rename it), and the second is friending the new followers. By pulling the filtering into new_followers, I can rename friend_all_followers to a more explicit friend_new_followers like this:

def old_followers # ... get old followers end def new_followers client.followers.reject { |f| old_followers.include?(f) } end def friend_new_followers new_followers.each do |follower| friend = Friend.create(:screen_name => follower.screen_name) client.friendship_create(friend.screen_name, true) end end