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


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

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

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

    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)

And I use methods from it like this

  class Tag
    include Search
    include Resource

    def initialize(tag_id = nil)
      @tag_id = tag_id

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

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.


1 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

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

  • \$\begingroup\$ Thanks a lot for your answer. I appreciate and will take all your recommendations into account. \$\endgroup\$ Jun 16, 2017 at 10:20
  • \$\begingroup\$ @SergeyMell no problem :) \$\endgroup\$ Jun 16, 2017 at 16:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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