1
\$\begingroup\$

I'm a little unhappy with the look of this "flexible" way of dealing with errors from an API in Ruby:

class ApiError < StandardError
  def initialize(response)
    message = response.is_a?(String) ? response
            : !(response.respond_to? 'body') ? 'Error'
            : response.body.is_a?(Hash) ? response.body['message']
            : response.body.to_s
    super(message)
  end
end

The idea is you can pass to ApiError.new either a string, or an object with an accessor called body in which case we'll take the body if it's a string or its message property if it's a hash. We'll use a generic error message if the argument is anything else.

What would be the idiomatic alternative, if any? I'm happy to take suggestions for completely different approaches (multiple constructors, static factories, etc.) but am still interested in a single-initialize-method approach. I know I can do if...elsif...elsif...else...end. Is that better?

\$\endgroup\$
  • \$\begingroup\$ I get a syntax error with your code. Maybe some brackets help. \$\endgroup\$ – knut Oct 12 '16 at 20:15
3
\$\begingroup\$

I would try a sequence of case statements:

class ApiError < StandardError
  def initialize(response)
    message = case
      when response.is_a?(String)
          response 
      when !(response.respond_to? 'body')
        'Error'
      when response.body.is_a?(Hash)
        response.body['message']
      else
        response.body.to_s
      end
    super(message)
  end
end

###test code
class XX
  attr_accessor :body
end

p ApiError.new('string')          
p ApiError.new(:x)          

xx = XX.new()
p ApiError.new(xx)
xx.body={'message' => 'my message'}
p ApiError.new(xx)

I hope I understood your code correct. Actually there is no check, if a given Hash has a 'message'-key.

You can also omit the message variable:

class ApiError < StandardError
  def initialize(response)
    super case
      when response.is_a?(String)
          response 
      when !(response.respond_to? 'body')
        'Error'
      when response.body.is_a?(Hash)
        response.body['message']
      else
        response.body.to_s
      end
  end
end
\$\endgroup\$
  • \$\begingroup\$ Agreed that the message-key check is good... when response.body.is_a?(Hash) && response.body.has_key?('message') \$\endgroup\$ – Ray Toal Oct 12 '16 at 20:37
  • \$\begingroup\$ Good answer, just two comments. 1) This is not the idiomatic case indentation. IMO it's ok to use it, though, maybe adding a note? 2) Mixing calls with parens and calls without parens does not look nice. \$\endgroup\$ – tokland Oct 13 '16 at 7:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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