current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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.

share|improve this question

2 Answers 2

up vote 5 down vote accepted

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
share|improve this answer
    
Yup, friendship_create is used to create the friendship in twitter. –  Gavin Miller May 18 '11 at 15:31
    
Is there any way to simplify the loop as well? –  Gavin Miller May 18 '11 at 15:33
    
Please see my update. Not sure where to go from there .) –  Eimantas May 18 '11 at 16:22
    
Getting rid of the call to save by using create only works if friendship_create doesn't do anything that needs to be saved. –  sepp2k May 18 '11 at 16:23
    
@sepp2k - agreed, but since this is API call - there is no need for it to modify the passed in object. –  Eimantas May 18 '11 at 16:24
up vote 1 down vote

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
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.