Proposal : Use Class for writing api wrappers

[redtachyons]

Context : We are currently recommending module for making making wrapper for external apis . It is making code difficult to maintain

Cons of module based approach

1. Not object oriented
2. When we are using module mixin and use it in other controllers , models etc , ruby copies the entire code and hence it will take more memory
3. Difficult to use multiple credentials of same provider , because we are storing global module self scope .
4. Method name will overlap when using as mixin

Alternative implementation using class

class Wufoo
  include HTTParty
 base_uri 'http://api.provider.com'
  attr_accessor :domain,:api_key
  def initialize(oprions={})
    @domain = options[:domain]
    @api_key = options[:api_key]
  end
   def leads
     # Logic here
   end
end

class Business < ActiveRecord::Base
 def wufoo
    Wufoo.new(domain,api_key)
  end
end

business = Business.find(27)
business.wufoo.leads

Advantages

1. Solving all the above problems
2. Easy write test
3. easy to cache
4. Easy to test from console

Ref : https://github.com/redpanthers/red-book/blob/master/_entries/5015-api-wrappers.md

[coderhs]

When we are using module mixin and use it in other controllers, models etc, ruby copies the entire code and hence it will take more memory

I am not sure about that. Module with self address and as we are only using CONSTANTS. Ruby won't copy them un-necessarily as they are not being included in the class. It's already loaded to memory with the app loads.

Classes, on the other hand, creates more objects, as they have a local memory.

I agree in the case of usage of an API for per user or business specific cases. So I guess we need to use a factory pattern or create classes and objects as you said.

But when working with command line applications or stuff like that. I think using the module approach is much cleaner

Method name will overlap when using as mixin

This is why I prefer to address them through the self method. Reader.start instead of being included into the code. So the method name overlap and confusion is avoided.

module PgJSON
  module Geneator
    def self.json_agg query:
      response_query = %{
        select json_agg(results) as response from (#{query}) as results
      }
      ActiveRecord::Base.connection.execute(response_query).each.first['response']
    end
  end
end

[coderhs]

I think your proposal needs to be added as a scenario. As the existing module approach will fail for the example you mentioned.

[redtachyons]

I am not sure about that. Module with self address and as we are only using CONSTANTS. Ruby won't copy them un-necessarily as they are not being included in the class. It's already loaded to memory with the app loads.

Ruby will copy the code when we are using it as mixins , Eg

class Controller1
  include FancyModule 
end

class Controller2
  include FancyModule
end

Here the contents of fancy module will be copied to both controllers

[redtachyons]

Classes, on the other hand, creates more objects, as they have a local memory.

We will be creating objects only when it is needed , and it will get Garbage collected after it's use

[redtachyons]

I agree in the case of usage of an API for per user or business specific cases. So I guess we need to use a factory pattern or create classes and objects as you said.

Do we really need that ? I think just using class object model is enough , check the snippet I posted above

[redtachyons]

This is why I prefer to address them through the self method. Reader.start instead of being included into the code. So the method name overlap and confusion is avoided.

We can do the same with class , but the problem is everything will be in global scope . But when we have to change attribute of a particular object , For Eg : Getting leads data using Wufoo api of a particular business
module Based approach

module Wufoo
  class << self
    attr_accessor :domain,api_key
   end
end
Wufoo.api_key = ""
Wufoo.domain = ""

Class based approach

class Wufoo
  attr_accessor :api_key,:domain
  def initalize(api_key,domain)
    @api_key,@doamin = api_key,domain
  end
end
wufoo = Wufoo.new("","")

Second approach is cleaner and difficult override and misuse

[coderhs]

Ruby will copy the code when we are using it as mixins , Eg

You haven't read my doc this it is mentioned that

Do not include and use the module directly, all the module through self as it would be more readable

You will be breaking our policy if you include.

[coderhs]

We will be creating objects only when it is needed, and it will get Garbage collected after it's use

Let's trust the garbage collector when its good enough. For now Ruby doesn't have the best Garbage collector (unless we use JRuby). We always need to reduce the number of objects being made. Unless we tune the GC and interpreter.

[coderhs]

I agree in the case of usage of an API for per user or business specific cases. So I guess we need to use a factory pattern or create classes and objects as you said.

Do we really need that ? I think just using class object model is enough , check the snippet I posted above

Yes, I agree to use your snippet. Only in that case. when we need to change the API credentials or data per user.

[redtachyons]

Let's trust the garbage collector when its good enough. For now Ruby doesn't have the best Garbage collector (unless we use JRuby). We always need to reduce the number of objects being made. Unless we tune the GC and interpreter.

Compared to the objects are being made by active record , this is lightweight and will be created only when we are calling that method , and it is easily cachable . And GC in ruby isn't is bad as it sounds

[redtachyons]

Yes, I agree to use your snippet. Only in that case. when we need to change the API credentials or data per user.

Thanks , but same thing is applicable for most of the apis we are using need these

Class GoogleAnalytics
  attr_accessor :start_date,:end_date
end
GoogleAnalytic.new(start_date: start_date, end_date: end_date).results

is much better than the init hack currently we use

[coderhs]

lightweight

Yes. But for a heavy volume application, this could lead to a spike in memory. Addressing to self-method in a module are a single object. No matter if its from just 10 requests or 10000 requests. I don't see the advantage of using class based design for use case which doesn't require one.

We shouldn't use a persistent object unless we do need one.

is much better than the init hack currently we use

The way it's used in clickx and way we defined in the document is different.

[coderhs]

Okay. here I am going to add all my comments for your problem in a single post.

Cons of module based approach

Not object oriented

Agreed.

When we are using module mixin and use it in other controllers, models etc, ruby copies the entire code and hence it will take more memory

Do not include the module, its mentioned there in our doc.

Difficult to use multiple credentials of same provider , because we are storing global module self scope .

Agreed. But let's say that there is a global provider. Like Seo Rush. Who has a single Key for us and we use it to get the search volume and CPC. As per your approach, we will either end up creating an object for each keyword, so that's like. If an organization has 900 keywords (which we do), we will end up creating 900 objects. Or if we create a single object per business then also we are creating more objects on each business. Which I feel like unnecessarily and our present system avoid.

Again. I agree when we get multiple credentials of the same provider in which case your approach is more suitable. I agree that we should add class based approach under such an example.

Method name will overlap when using as mixin

Again don't include or exclude the module. Its meant to be used as Majestic.search keyword: 'keyword'

Going through the advantage you mentioned:

Advantages

Solving all the above problems

Except one all others should not rise as a problem.

Easy write test

I am not sure what you meant by this one. First of all, we limit the modules to do only API fetch/update or format of data as per our requirement so we stub all the API request and process the subbed responses. So we are testing only if the output we expect is being received for the input we send. We won't be testing the changes it makes to our system here, that has to be done in the method where it is called.

easy to cache

Again not sure what you meant here, as we shouldn't be caching within the methods of the module. Like not calling ActiveRecord from inside the module. It goes for all the rest, no caching within in the module. We cache the method from which the request is made.

Easy to test from console

There is no problem with testing it from the console. You just need to write Majestic.search keyword: 'keyword'.

---

Personally, the only thing I can agree to is to use this approach when there are multiple credentials for single service. Where we can't avoid creating multiple objects as by definition they would be multiple objects.