Is it cute or ugly?

… and check why 5600+ Rails engineers read also this

Is it cute or ugly?

Yesterday day I presented a small code snippet to my dear coworkers (who I of course always respect) and (as usually) they were not impressed, even though I liked my solution. So I wanted to know your opinion about this little piece of code. Let’s start with the beginning.

Obvious way

The story is simple. You are writing code to communicate with external api. You are thoughtful programmer so you don’t store credentials in the code. You deploy to Heroku so obviously you keep things in ENV.

First thought:

class ApiProvider
  def initialize(login = nil, password = nil)
    login    ||= ENV['APIPROVIDER_LOGIN']
    password ||= ENV['APIPROVIDER_PASSWORD']
    @uri       = Addressable::URI.parse("http://api.example.org/query")
    @uri.query_values = {usr: login, pwd: password}
  end
end

My immediate concern: Why should the instance of my class know in its constructor about the fact that we use ENV to store default login and password values. So perhaps we should use some kind of factory that would create the object with provided values or the defaults ?

But in Ruby every class is a factory, so why not use is to our advantage…

Introduce Factory


class ApiProvider
  def self.new(login = nil, password = nil)
    login    ||= ENV['APIPROVIDER_LOGIN']
    password ||= ENV['APIPROVIDER_PASSWORD']
    super(login, password)
  end

  def initialize(login, password)
    @uri = Addressable::URI.parse("http://api.example.org/query")
    @uri.query_values = {usr: login, pwd: password}
  end
end

There is still something wrong here, I think. Why would anyone want to provide login, but not password ? Or password without login ? Doesn’t make much sense to me. So I decided to extract a new, little class.

Extract class

class ApiProvider
  class Credentials < Struct.new(:login, :password)
  end

  def self.new(credentials = nil)
    credentials ||= Credentials.new( ENV['APIPROVIDER_LOGIN'], ENV['APIPROVIDER_PASSWORD'] )
    super(credentials)
  end

  def initialize(credentials)
    @uri = Addressable::URI.parse("http://api.example.org/query")
    @uri.query_values = {usr: credentials.login, pwd: credentials.password}
  end
end

Does it make sense here to use Struct ? I think so, because Credentials.new('l', 'p').should == Credentials.new('l', 'p'). But there are coworkers who disagree with me and I wonder what you think.

Alternatives

  • Default in method definition
class ApiProvider
  def self.new(credentials = Credentials.new( ENV['APIPROVIDER_LOGIN'], ENV['APIPROVIDER_PASSWORD'] ))
    super
  end
end

# or

class ApiProvider
  def initialize(credentials = Credentials.new( ENV['APIPROVIDER_LOGIN'], ENV['APIPROVIDER_PASSWORD'] ))
    @uri = Addressable::URI.parse("http://api.example.org/query")
    @uri.query_values = {usr: credentials.login, pwd: credentials.password}
  end
end

Somehow this seems to be less readable to me

  • Moving the defaults to Credentials
class ApiProvider
  class Credentials < Struct.new(:login, :password)
    def self.default
      new( ENV['APIPROVIDER_LOGIN'], ENV['APIPROVIDER_PASSWORD'] )
    end
  end

  def initialize(credentials = Credentials.default)
    @uri = Addressable::URI.parse("http://api.example.org/query")
    @uri.query_values = {usr: credentials.login, pwd: credentials.password}
  end
end

Nice, but the knowledge about defaults was transffered from ApiProvider.new factory method to Credentials and I believe that Credentials should but just a dumb class responsible only for keeping login and password always together. Because in terms of this api it never makes sense to operate separately on them.

  • External context is always responsible for providing the configuration
api = ApiProvider.new( ENV['APIPROVIDER_LOGIN'], ENV['APIPROVIDER_PASSWORD'] )
api.do_something

This leads to repeated code if there are multiple places that need to instantiate ApiProvider.

TLDR

  • The constructor states that ApiProvider always requires credentials as dependency for proper working
class ApiProvider
  def initialize(credentials)
    @uri = Addressable::URI.parse("http://api.example.org/query")
    @uri.query_values = {usr: credentials.login, pwd: credentials.password}
  end
end
  • ApiProvider.new factory method is responsible for creating ApiProvider instance even without explicit credentials because defaults can be used.
class ApiProvider
  def self.new(credentials = nil)
    credentials ||= Credentials.new( ENV['APIPROVIDER_LOGIN'], ENV['APIPROVIDER_PASSWORD'] )
    super(credentials)
  end
end
  • Credentials is just a dumb struct for passing login and password together around the system
class Credentials < Struct.new(:login, :password)
end

Which way do you like the most ? Do you agree with me ? Or have I just earned uncountable amount of haters ? Do you think that using Struct is a bad practice sometimes ?

You might also like