Ruby - class definition for connecting to external API

Question

I've defined a Client class to connect to an external API. The Client class will return XML responses for various resources. Here's what it looks like now:

require 'nokogiri'

class Client
  include Exceptions

  CONFIG = YAML.load_file(File.join(Rails.root, 'config', 'mark.yml'))

  def self.get_leagues_xml(league, start_date, end_date)
    raise ArgumentError, "No args can be nil." if league.nil? || start_date.nil? || end_date.nil?

    start_date = format_date(start_date)
    end_date = format_date(end_date)

    @url = build_url_for(:leagues)
    @url_params = {'League' => league, 'StartDate' => start_date, 'EndDate' => end_date}

    call_http_service
  end

  def self.get_seasons_xml(season)
    raise ArgumentError, "No args can be nil." if season.nil?

    @url = build_url_for(:seasons)
    @url_params = {'Season' => season}

    call_http_service
  end

  protected

  def self.call_http_service
    begin
      conn = create_connection
      resp = get_response(conn)
      resp_xml = Nokogiri::XML(resp.body)
    rescue Faraday::Error::ClientError => ce
      raise MarkClientError.new("Client error occurred trying to access Mark feed at #{@url}: #{ce.message}.")
    rescue => e
      raise e
    end
  end

  def self.get_response(connection)
    connection.get do |req|
      req.params['Username'] = CONFIG['api']['username']
      req.params['Password'] = CONFIG['api']['password']
      @url_params.each_pair { |key, val| req.params[key] = val}
      req.options = {:timeout => 30} # open/read timeout in secs
    end
  end

  def self.create_connection
    Faraday.new(:url => @url) do |builder|
      builder.request  :url_encoded
      builder.response :logger
      builder.adapter  :net_http
    end
  end

  def self.build_url_for(resource)
    CONFIG['api']['url'] + CONFIG['resource']["#{resource.to_s}"]['url']
  end

  def self.format_date(date)
    date = Chronic.parse(date) if date.is_a?(String)
    date.strftime('%m/%d/%Y')
  end

end

My primary question is whether or not my use of the class instance variables (@url and @url_params) is an anti-Rubyism and will get me into trouble when multiple concurrent class method calls are being made? I've considered making Client a module or a superclass and then having separate classes for each type of API call that needs to made, for ex, LeaguesClient and SeasonsClient. That way I could make those instantiated classes and initialize with url and url_params. I'm starting to experiment with the distinct nuances of Ruby but want to check with you guys as to how you would approach. Any thoughts you have would be welcome. Thanks!

Answer 1

CONFIG = YAML.load_file(File.join(Rails.root, 'config', 'mark.yml'))

This does not belong inside the class, it makes it impossible for others to change the configuration depending on environment (dev, test, prod). You can either pass in a map of options to the constructor or say in Rails use an initializer class to read in the appropriate configuration and apply it.

raise ArgumentError, "No args can be nil." if league.nil? || start_date.nil? || end_date.nil?

It's good to validate your arguments, however this message doesn't help the developer/user of the class because it doesn't indicate which argument was nil. As tedious as it is I would validate each arg separately and indicate which argument was nil in the message.

Regarding the use of @url and @url_params sgmorrison is correct and you should not be using class instance variables especially in a static class. If their values were immutable then fine but that is not the case. A simple change would be to pass both url and urlparams as options/args to call_http_service and so forth.

Another option would be to separate the two concerns of this class into individual classes. One that is a service class and one that is a connection class. Then you can encapsulate all connection logic into one (likely none static) class, and the user can interface with the service class. Should you choose this method you could then use a factory and/or mock out the connection to make unit testing easier.

Answer 2

Using the @url and @url_params variables this way is not idiomatic Ruby. There are similar uses of instance fields in Ruby on Rails, but this is for passing state out of Controllers and into Views. It is not standard object-oriented practice, but in Rails it is done to remove boiler-plate code. Elsewhere this style is not the norm.

You should also be consistent within the class in how you pass state between methods. get_response expects a connection to be passed to it, but pulls another "parameter" @url_params out of the class-level fields. It should get all input from one location to make it easier to follow the code.