Extracting ids out of arguments of different types

Question

I'm building a Ruby API client. My brief is to extract ids out of various inputs and fetch the relevant data. It's working just fine at the moment but I think it's a little clumsy.

My specs (which pass):

shared_examples 'it requests ids' do |resource|
 it 'sends the ids to Foo::Client' do
 Foo.stub(:parse)
 Foo::Client.should_receive(resource).once.with(expected)
 Foo.send(resource, *args)
 end
end
describe Foo do
 describe 'resources have numeric ids only'
 [:accounts, :contacts].each do |resource|
 it_behaves_like 'it requests ids', resource do
 let(:args) { [1, 2] }
 let(:expected) { [1, 2] }
 end
 it_behaves_like 'it requests ids', resource do
 let(:args) { [3, 'jesse-pinkman', 5, {'links' => {resource.to_s => [98, 99]}}] }
 let(:expected) { [98, 99, 3, 5] }
 end
 end
 end
 describe 'resources have numeric and string ids'
 [:clients, :services].each do |resource|
 it_behaves_like 'it requests ids', resource do
 let(:args) { [2,'saul-goodman', 6] }
 let(:expected) { [2, 6, 'saul-goodman'] }
 end
 it_behaves_like 'it requests ids', resource do
 let(:args) { [3, 'saul-goodman', 5, {'links' => {resource.to_s => [202, 234]}}] }
 let(:expected) { [202, 234, 3, 5, 'saul-goodman'] }
 end
 end
 end
end

And my code:

module Foo
 class << self
 [:accounts, :contacts, :clients, :services].each do |resource|
 define_method(resource) do |*args|
 ids = []
 unless args.empty?
 ids += hash_ids(args, resource)
 ids += numeric_ids(args)
 ids += text_ids(args) unless numeric_ids_only?(resource)
 end
 parse(client.send(resource, ids))
 end
 end
 private
 def hash_ids(args, resource)
 args.collect { |arg| arg['links'][resource.to_s] if arg.is_a? Hash }.compact!.flatten!.to_a
 end
 def numeric_ids(ids)
 ids.select { |id| !id.is_a?(Hash) && id.to_i > 0 }
 end
 def text_ids(ids)
 ids.select { |id| id.is_a? String }
 end
 def numeric_ids_only?(resource)
 [:accounts, :contacts].include?(resource)
 end
 def client
 Foo::Client
 end
 def parse(json)
 Foo::Adapter::JSON.parse(json)
 end
 end
end

My particular concern is the ugly concatenating of the ids array and passing all those args around. Any tips?

Answer 1

You should expect
Starting a couple of years back, rspec is moving to a new expectation syntax, you should consider porting to it:

shared_examples 'it requests ids' do |resource|
 it 'sends the ids to Foo::Client' do
 allow(Foo).to receive(:parse)
 expect(Foo::Client).to receive(resource).once.with(expected)
 Foo.send(resource, *args)
 end
end

Be more transparent
Your code hides pretty well the fact that clients and services is different from accounts and contacts - both in your code and in your tests.
It makes your code quite obfuscated (in the name of DRYness?) - you should make it clear that you have two different behaviors - and spell them out:

shared_examples 'resources have numeric ids only' do |resource|
 it_behaves_like 'it requests ids', resource do
 let(:args) { [1, 2] }
 let(:expected) { [1, 2] }
 end
 it_behaves_like 'it requests ids', resource do
 let(:args) { [3, 'jesse-pinkman', 5, {'links' => {resource.to_s => [98, 99]}}] }
 let(:expected) { [98, 99, 3, 5] }
 end
end
shared_examples 'resources have numeric and string ids' do |resource|
 it_behaves_like 'it requests ids', resource do
 let(:args) { [2,'saul-goodman', 6] }
 let(:expected) { [2, 6, 'saul-goodman'] }
 end
 it_behaves_like 'it requests ids', resource do
 let(:args) { [3, 'saul-goodman', 5, {'links' => {resource.to_s => [202, 234]}}] }
 let(:expected) { [202, 234, 3, 5, 'saul-goodman'] }
 end
end
it_behaves_like 'resources have numeric ids only', :accounts
it_behaves_like 'resources have numeric ids only', :contacts
it_behaves_like 'resources have numeric and string ids', :clients
it_behaves_like 'resources have numeric and string ids', :services

And in your code, I would rather use simple method delegation than meta-coding:

class << self
 def accounts(*args)
 parse(client.accounts(hash_ids(args, :accounts) + numeric_ids(args))
 end
 def contacts(*args)
 parse(client.accounts(hash_ids(args, :contacts) + numeric_ids(args))
 end
 def clients(*args)
 parse(client.accounts(hash_ids(args, :clients) + numeric_ids(args) + text_ids(args))
 end
 def services(*args)
 parse(client.accounts(hash_ids(args, :services) + numeric_ids(args) + text_ids(args))
 end
end

It might be less DRY, but it is a lot clearer what your class actually does.

• Rimian: Nice. I have refactored my specs to clarify the behaviour a little. I think you're right.

Answer 2

Rimian, the main suggestions I would make are two:

• loop through args just once; and

• use a case statement with argument arg, as it makes it easy to separate the arg objects by class.

Here is one way to apply these suggestions:

Code

require 'json'
module Foo
 class Client
 class << self
 [:accounts, :contacts, :clients, :services].each { |resource|
 define_method(resource) { |*args| process(resource, args) } }
 private
 def process(resource, *args)
 parse(client.send(resource, ids(resource, args)))
 end
 def ids(resource, *args)
 args.each_with_object([]) do |arg,a|
 case arg
 when Hash
 v = arg['links'][resource.to_s]
 v.each { |e| a << e } unless v.nil?
 when Numeric
 a << arg
 when String
 a << arg unless [:accounts, :contacts].include?(resource)
 end
 end
 end
 def client
 Foo::Client
 end
 def parse(json)
 Foo::Adapter::JSON.parse(json)
 end
 end
 end
end

Examples

Actually, I am not 100% certain this is working, because I am not familiar with JSON, but I expect it will not be difficult to repair if there are errors.

I did a poor-man's test by commenting-out private and running:

resource = :accounts
p Foo::Client.ids(resource, 1, 2)
 #=>[1, 2]
resource = :contacts
p Foo::Client.ids(resource, 3, 'jesse-pinkman', 5,
 {'links' => {resource.to_s => [98, 99]}})
 #=> [3, 5, 98, 99]
resource = :clients
p Foo::Client.ids(resource, 2,'saul-goodman', 6)
 #=> [2, 6, 'saul-goodman']
resource = :services
p Foo::Client.ids(resource, 3, 'saul-goodman', 5,
 {'links' => {resource.to_s => [202, 234]}})
 #=> [202, 234, 3, 5, 'saul-goodman']

Explanation and observations

• Where is your Client class? I assume from the spec that it is to be defined in the module Foo, rather than Client including Foo, but that's a detail. Also, you forgot require 'json'. That's obvious, but it's still a good idea to include all requires when showing your code.

• I didn't know if you intended to include non-positive numeric values, or that you expect them all to be positive and was using arg.to_i > 0 to distinguish them from strings. If you want to exclude non-positive Fixnums, add if arg > 0 in the appropriate place.

• You will recall that case statements use Object#=== rather than == for determining true and false, which is why, for example, when Hash works.

• I don't think you actually need your client method, as self should be Foo::Client whenever parse(send(resource, ids(resource, args))) is invoked.

• Rather than using define_method to create four class methods, you could simply make my method process public and pass the resource as an additional argument; for example: Foo::Client.process(:accounts, 1, 2). Not advocating, just mentioning.

• Rimian: Good suggestions. I think your ids method might have too many levels of abstractions however. What do you think? I have a preference for short methods that do only one thing.