4
\$\begingroup\$

I have an http library in Ruby with a Response class, I have broken it down into smaller methods to avoid having a big intialize method, but then I end up with this:

 def initialize(res)
    @res = res
    @headers = Hash.new

    if res.empty?
      raise InvalidHttpResponse
    end

    split_data
    parse_headers
    set_size
    check_encoding
    set_code
  end

I don't feel well about calling function after function like that, any suggestions on how to improve this? Full code is at github.

\$\endgroup\$
5
\$\begingroup\$

Looking through your whole module it's apparent you've only had experience with imperative programming (it's full of side-effects, variable re-bounds, in-place updates, ...). I've already written on the topic (see this and this) so I won't expand here about the goodness of functional programming.

I'd write:

def initialize(response_string)
  @code = parse_code(response_string)
  raw_headers, @body = parse_data(response_string)
  @headers = parse_headers(raw_headers)
end

You get the idea, create parsing methods (or class methods, actually they won't need instance variables, they are pure functions) with inputs (arguments) and outputs that get assigned/bound to instance variables just once (not incidentally, that makes those methods easier to test).

On the other hand, there are already good network libraries for Ruby, what's the point of re-implementing them?

[EDIT] You asked for further advice. Ok, let's get a couple of methods of Response and refactor them:

def parse_data(raw_data)
  all_headers, body = raw_data.split("\r\n\r\n", 2)
  raise InvalidHttpResponse unless body && raw_data.start_with?("HTTP")
  raw_headers = all_headers.split("\r").drop(1)
  [raw_headers, body]
end

def decode_data(body)
  # You can use condition ? value1 : value2 for compactness
  data = if headers["Transfer-Encoding"] == "chunked"
    decode_chunked(body)
  else
    body
  end

  if headers["Content-Encoding"] == "gzip" && body.length > 0
    Zlib::GzipReader.new(StringIO.new(data)).read
  else
    data
  end
end

Ideas behind the refactor:

  • Put a space after a comma (more on style).
  • Don't reuse variables names (different values must have different names).
  • Don't update variables in-place.
  • Don't write explicit return.
  • Use if conditionals as expressions.
  • Use || && for boolean logic.
  • Put parentheses on calls (except on DSL code).
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you, I just pushed the changes if you want to check, any further advice? \$\endgroup\$ – matugm Mar 31 '13 at 2:59
  • \$\begingroup\$ @matugm: more on the answer. \$\endgroup\$ – tokland Mar 31 '13 at 9:51

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.