HTTP client for company services

Question

For quite a while now I've been writing a bit of Ruby code, but so far I still don't think it was good idiomatic Ruby. But there's been a new requirement and I tried my best writing it better, more cleanly compared to my previous code. One of the things I did is read another code base that I though was well written (calabash-ios).

I would like to hear your opinions and suggestions for improvements.

module Company
  module MobileServices
    class Client
      include DigestHelper

      attr_reader :client_name, :client_digest_key

      # headers that will be part of every request type 
      REQUEST_INIT_HEADER = {
          'User-Agent' => 'Calabash/MobileServicesClient',
          'Accept' => 'application/json'        
      }

      # when creating a new instance, make sure the client name and digest key 
      # are correct as a digest key is linked to a specific client
      def initialize(client_name = 'client_x', client_digest_key = 'some_random_key')
        @client_name = client_name       
        @client_digest_key = client_digest_key 
      end

      # execute request base on path; optionally add the following parameters
      # - user_id: a registered email address for the target environment (host)
      # - password: the password linked to the email address
      # - host: default is some.host.nl, change as needed
      # - http_method: GET (default), PUT, POST or DELETE are supported
      # - body: the body of the request, only JSON content is supported
      #
      # PLEASE NOTE: we can't get URL's through proxy working, digest 
      # calculation seems to fail
      def execute_request(path, options={})
        default_options = {:host => 'some.host.nl',
                           :user_id => '[email protected]',
                           :password => 'pa55w0rd',
                           :http_method => 'GET',
                           :body => ''}

        options = default_options.merge(options)

        digest = calc_digest(path, options[:user_id], @client_digest_key, options[:body])
        uri = URI.parse("https://" + options[:host] + path + "?digest=#{digest}")

        # create a request and add the default headers like authorization and host        
        http_request = new_http_request(uri, options[:http_method], options[:body])
        add_default_headers(http_request, options[:host], options[:user_id], options[:password])

        # create a new http client that we'll use to execute the request
        http_client = new_http_client(uri, digest)

        # execute request & print results
        puts "\n#{uri}\n\n"
        puts "#{http_request.method} #{http_request.path}"
        http_request.each_header {|key,value| puts "#{key} = #{value}" }
        puts "\n"
        http_response = http_client.request(http_request)
        puts "#{http_response.code} #{http_response.message}\n"
        puts "#{http_response.body}" unless http_response.body.nil?
        puts "\n--------------------------------------------------------------\n"
      end

  private #---------------------------------------------------------------------

      # create a new http client and in case of https we mark it as SSL, but we
      # wont verify the certificates
      def new_http_client(uri, digest)
        http_client = Net::HTTP.new(uri.host, uri.port)

        if uri.scheme == 'https'
          http_client.use_ssl = true
          http_client.verify_mode = OpenSSL::SSL::VERIFY_NONE
        end

        http_client
      end

      # create a new http request with an optional body; when a body exists, we 
      # assume it's JSON content and add the appropriate header
      def new_http_request(uri, http_method, body = '')
        http_request = nil

        # depending on the http method type, create the appropriate request type
        case http_method
        when 'GET'
          http_request = Net::HTTP::Get.new(uri.request_uri, REQUEST_INIT_HEADER)
        when 'PUT'
          http_request = Net::HTTP::Put.new(uri.request_uri, REQUEST_INIT_HEADER)
        when 'POST'
          http_request = Net::HTTP::Post.new(uri.request_uri, REQUEST_INIT_HEADER)
        when 'DELETE'
          http_request = Net::HTTP::Delete.new(uri.request_uri, REQUEST_INIT_HEADER)
        else 
          raise "invalid or unsupported HTTP method: #{http_method}"
        end

        # we'll assume body content is always JSON, so if we have a body we'll
        # add the content type header
        unless body == ''
          http_request.add_field("Content-Type", "application/json") 
          http_request.body = body
        end

        http_request
      end

      # adds the following headers to a request:
      # - host: the server we're connecting with
      # - client-name: the client that is linked to our digest
      # - authorization: a hashed value based on user id and password
      def add_default_headers(http_request, host, user_id, password)
        unless http_request.nil?
          http_request.basic_auth(user_id, password)
          http_request.add_field("Client-Name", @client_name)
          http_request.add_field("Host", host)           
        end

        http_request
      end

    end
  end
end

---

module Company
  module MobileServices
    module DigestHelper

      def calc_digest(path, user_id, digest_key, body='')
        body = body.bytes[0..1000].pack('c*')
        text = path + body + user_id + digest_key
        digest = Digest::SHA1.digest(text)
        hex_digest = digest.unpack('H*').first.downcase

        hex_digest
      end

    end
  end
end

Answer 1

In (subjective) order or relevance:

---

You shouldn't really raise a RuntimeError (default):

raise "invalid or unsupported HTTP method: #{http_method}"

Instead, define your own exception class and raise it:

HttpMethodError = Class.new(StandardError)
...
raise HttpMethodError, "invalid method #{method}"

RuntimeError is pretty generic, and if something else throws it, your rescue statements could not tell the difference, making it hard to handle such exception.

---

Your methods are somewhat long thus unreadable. Find atomic tasks and extract them, i.e.

  def request_for_method http_method
    case http_method
    when 'GET'
      Net::HTTP::Get.new(uri.request_uri, REQUEST_INIT_HEADER)
    when 'PUT'
      Net::HTTP::Put.new(uri.request_uri, REQUEST_INIT_HEADER)
    when 'POST'
      http_request = Net::HTTP::Post.new(uri.request_uri, REQUEST_INIT_HEADER)
    when 'DELETE'
      Net::HTTP::Delete.new(uri.request_uri, REQUEST_INIT_HEADER)
    else 
      raise "invalid or unsupported HTTP method: #{http_method}"
    end
  end

You could also split execute_request in at least two parts (logic in first, putses in second.

---

Newer version's of Ruby (2.0 up I think) would allow you to turn this:

  def execute_request(path, options={})
    default_options = {:host => 'some.host.nl',
                       :user_id => '[email protected]',
                       :password => 'pa55w0rd',
                       :http_method => 'GET',
                       :body => ''}

into this:

def execute_requests(path, host: 'some.host.nl', user_id: '[email protected]' ...)

---

There's no need to check for nil explicitly:

unless http_request.nil?
# equals
if http_request

Likewise:

unless body == ''
# equals
unless body.empty?

Some people would argue that second option is more readable.

---

This is more like a hint, or an opinion than an issue: if you have such deeply nested code you can do things like:

class Company::MobileServices::Client
  include DigestHelper
  # ...
end

While I'd say it makes your code more readable (provided you only have single class in a file), in Ruby we most often only use two spaces to indent, so it's not really a problem.

---

Even if you don't extract this case in it's method (or if you do), don't assign to variable in each when. Ruby way to do this is to leverage the fact that case is an expression returning a value (like everything else is):

    http_request =
      case http_method
      when 'GET'
        Net::HTTP::Get.new(uri.request_uri, REQUEST_INIT_HEADER)
      when 'PUT'
        Net::HTTP::Put.new(uri.request_uri, REQUEST_INIT_HEADER)
      when 'POST'
      # ...
      end