patternMinor
Pattern matching deeply nested arguments in Elixir
Viewed 0 times
argumentsdeeplynestedelixirpatternmatching
Problem
I've recently started working with Elixir and am writing an app that queries various URLs and parses the response. I am having issues with my
This is the code that sets the whole thing in process. I think it's fine, but include it to give you some background:
The
Here is the
My biggest objection is really the size of the
How can the
parse method, which is included below. The code works just fine, but I am concerned that I may have taken pattern matching too far. The method signature looks like spaghetti.This is the code that sets the whole thing in process. I think it's fine, but include it to give you some background:
def run(job) do
HTTPStatusCheck.query(job)
|> HTTPStatusCheck.parse
|> update_db
|> send_alerts
endThe
query method returns this "object":%HTTPCheckResult{parsed: false, http_response: resp}
# resp is a %HTTPoison.Response{} "object"Here is the
parse method I need help with:def parse(%HTTPCheckResult{
parsed: false, http_response: {:ok, %Response{status_code: c}}} = result)
when c in @up do
%{result | parsed: true, code: c, result: :up}
end
def parse(%HTTPCheckResult{
parsed: false, http_response: {:ok, %Response{status_code: c}}} = result)
when c in @down do
%{result | parsed: true, code: c, result: :down}
end
def parse(%HTTPCheckResult{
parsed: false, http_response: {:ok, %Response{status_code: c}}} = result) do
%{result | parsed: true, result: :error, error: "http code #{c}"}
end
def parse(
%HTTPCheckResult{parsed: false, http_response: {:error, %HTTPoison.Error{reason: reason}}} = result) do
%{result | parsed: true, result: :error, error: reason}
end
def parse(%HTTPCheckResult{parsed: false} = result) do
%{result | result: :error, error: "unknown"}
endMy biggest objection is really the size of the
parse method signatures, which are three lines in some cases. The important parts also seem pretty deeply nested in there.How can the
parse method can be improved?Solution
I've just taken a whack at cleaning up my original code after a few days.
edit: Now including refactor suggestions from alxndr.
Set everything in motion
(The new
The new
(You can see it calls
The new
(I feel these are much better, as the method signature and definition code are now much shorter and more readable. The suggestions from alxndr were implemented here and in the next section of helper methods.)
Helper methods for
There is no longer the need to indicate
edit: Now including refactor suggestions from alxndr.
Set everything in motion
def run(job) do
HTTPStatusCheck.run(job)
|> update_db
|> send_alerts
end(The new
run method seen here handles what was previously done by query and parse.)The new
HTTPStatusCheck.run/1 method:def run(job) do
method = :head
url = job.url
body = ""
headers = [@user_agent]
options = @http_options
HTTPoison.request(method, url, body, headers, options)
|> parse
end(You can see it calls
parse internally now, as I cannot find a reason anything else would ever need to work with the unparsed results object.)The new
HTTPStatusCheck.parse/1 definitions:defp parse({:ok, %Response{status_code: c}} = resp) do
%HTTPCheckResult{}
|> add_http_code(c)
|> add_http_response(resp)
end
defp parse({:error, %HTTPoison.Error{reason: reason}} = resp) do
%HTTPCheckResult{}
|> add_http_response(resp)
|> add_error(reason)
end
defp parse(_) do
%HTTPCheckResult{}
|> add_error("unknown")
end(I feel these are much better, as the method signature and definition code are now much shorter and more readable. The suggestions from alxndr were implemented here and in the next section of helper methods.)
Helper methods for
HTTPStatusCheck.parse/1:defp add_result(result, val), do: %{result | result: val}
defp add_http_response(result, response), do: %{result | http_response: response}
defp add_http_code(result, code) do
%{result | code: code}
|> handle_http_code(code)
end
defp handle_http_code(result, code) when code in @up do
result
|> add_result(:up)
end
defp handle_http_code(result, code) when code in @down do
result
|> add_result(:down)
end
defp handle_http_code(result, code) do
result
|> add_error("unhandled http code #{c}")
end
defp add_error(result, error) do
%{result | error: error}
|> add_result(:error)
end%HTTPCheckResult{} is improved too:There is no longer the need to indicate
parsed: false or parsed: true on this struct.Code Snippets
def run(job) do
HTTPStatusCheck.run(job)
|> update_db
|> send_alerts
enddef run(job) do
method = :head
url = job.url
body = ""
headers = [@user_agent]
options = @http_options
HTTPoison.request(method, url, body, headers, options)
|> parse
enddefp parse({:ok, %Response{status_code: c}} = resp) do
%HTTPCheckResult{}
|> add_http_code(c)
|> add_http_response(resp)
end
defp parse({:error, %HTTPoison.Error{reason: reason}} = resp) do
%HTTPCheckResult{}
|> add_http_response(resp)
|> add_error(reason)
end
defp parse(_) do
%HTTPCheckResult{}
|> add_error("unknown")
enddefp add_result(result, val), do: %{result | result: val}
defp add_http_response(result, response), do: %{result | http_response: response}
defp add_http_code(result, code) do
%{result | code: code}
|> handle_http_code(code)
end
defp handle_http_code(result, code) when code in @up do
result
|> add_result(:up)
end
defp handle_http_code(result, code) when code in @down do
result
|> add_result(:down)
end
defp handle_http_code(result, code) do
result
|> add_error("unhandled http code #{c}")
end
defp add_error(result, error) do
%{result | error: error}
|> add_result(:error)
endContext
StackExchange Code Review Q#131502, answer score: 3
Revisions (0)
No revisions yet.