Ruby wrapper for Instagram API

Question

I've created my first gem as Instagram API wrapper https://github.com/agilie/instagram_api_gem. I would be very pleased if you give me some feedback on code organization, methods elegancy and so on.

The thing I concerned about the most is next. I have a module Resource

  module Resource
    include Client

    private

    def show(id, options = {})
      raise 'Resource ID is required' unless id
      make_request resource_path(id), query: options
    end

    def index(id, resource_name, options = {})
      raise 'Resource ID is required' unless id
      make_request resource_path("#{id}/#{resource_name}"), query: options
    end

    def create(id, resource_name, options = {})
      raise 'Resource ID is required' unless id
      make_request(resource_path("#{id}/#{resource_name}"), { body: options }, :post)
    end

    def destroy(id, resource_name, resource_id = nil, options = {})
      raise 'Resource ID is required' unless id
      path = "#{id}/#{resource_name}"
      path = "#{path}/#{resource_id}" if resource_id
      make_request(resource_path(path), { query: options }, :delete)
    end
  end

And I use methods from it like this

  class Tag
    include Search
    include Resource

    def initialize(tag_id = nil)
      @tag_id = tag_id
    end

    def recent_media(options = {})
      index @tag_id, 'media/recent', options
    end
  end

Does this implementation look good or are there more elegant solutions? I would be very pleased if you give me some feedback regarding the whole code organization and implementation, which could be found here https://github.com/agilie/instagram_api_gem

You are welcome to contribute if you prefer.

Answer 1

I find your code to be very good and readable :)

In the code you present here, I only see one mishaps :

raise 'Resource ID is required' unless id

is repeated multiple time and actually don't test for much. I'd consider making a private method that check id and also check if id is not empty (or even if it isn't strip.empty? to consider only-spaces strings).

For the create method, can the option parameters really can be an empty hash ? If not I'd consider at least removing the default value for this.

I haven't read your whole repo, so I'm not sure but does the Tag class really need to include both Search and Ressource ?

Code like

@user_id = user_id ? user_id : 'self'

should be replaced with @user_id = user_id || 'self'

One important point : your code must be hard to unit test as it is coupled with HTTParty, you should consider using a Strategy pattern here.

Finally, I'd make some simple class that have only one or two fields (such as Tag, Location, User...) immutable by duping and freezing the parameter and freezing the whole object : (note that it won't work if you expect a boolean, a number, a Symbol or nil as dup raise an exception in such case :()

def initialize(user_id)
  @user_id = (user_id || 'self').dup
  @user_id.freeze
  freeze
end

I haven't done Ruby in some time so I'm a bit rusty but I hope my review helped you at least a bit :)