HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Simplification of loop in Ruby

Submitted by: @import:stackexchange-codereview··
0
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:

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.

Solution

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

Code Snippets

friend = Friend.new(:screen_name => follower.screen_name)
client.friendship_create(friend.screen_name, true)
friend.save
friend = 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  
end
friend = 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
end

Context

StackExchange Code Review Q#2467, answer score: 7

Revisions (0)

No revisions yet.