Extracting ids out of arguments of different types

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?

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