patternrubyMinor
Refactor a sequence of functions
Viewed 0 times
functionsrefactorsequence
Problem
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:
I don't feel well about calling function after function like that, any suggestions on how to improve this? Full code is at github.
def initialize(res)
@res = res
@headers = Hash.new
if res.empty?
raise InvalidHttpResponse
end
split_data
parse_headers
set_size
check_encoding
set_code
endI don't feel well about calling function after function like that, any suggestions on how to improve this? Full code is at github.
Solution
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:
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
Ideas behind the refactor:
I'd write:
def initialize(response_string)
@code = parse_code(response_string)
raw_headers, @body = parse_data(response_string)
@headers = parse_headers(raw_headers)
endYou 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
endIdeas 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
ifconditionals as expressions.
- Use
||&&for boolean logic.
- Put parentheses on calls (except on DSL code).
Code Snippets
def initialize(response_string)
@code = parse_code(response_string)
raw_headers, @body = parse_data(response_string)
@headers = parse_headers(raw_headers)
enddef 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
endContext
StackExchange Code Review Q#24525, answer score: 5
Revisions (0)
No revisions yet.