Pattern matching deeply nested arguments in Elixir

Question

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 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
 end

The 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"}
 end

My 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?

• Scott Swezey: @200_success: I appreciate you taking the time to review and edit my question. Sadly, I feel you misunderstood the problem I was looking to solve. I've made additional edits to try and clarify the question. In short, the code works fine. I just need help refactoring the parse methods to make them easier to read and understand.
• 200_success: Your title in Rev 4 violates site guidelines (see How to Ask). "Refactoring long method" is way too generic a title for this site. In addition, you should be aware that all Code Review questions are considered requests for open-ended critique. You can ask a question if you want, but you might be disappointed if an answer picks on some other problem. Please feel free to stop by the chat room if you have questions about how Code Review works.
• Scott Swezey: I have no problems with open ended feedback on my code. I should note that I am not having any issues creating HTTP requests, nor handling those errors. I just want to make the code more readable. At this point, if my question remains "off base" for this "code review" site, I'll delete it and find somewhere else for assistance. Thank you for your help.
• Mathieu Guindon: FWIW 200 seems to merely be trying to make your title comply with what this community has agreed they want titles to say - what the code does, not what you want to fix about it. "State the task that your code accomplishes. Make your title distinctive." - see this meta for an explanation of why that's the case, and this meta if you need title ideas.

Answer 1

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

 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.

• alxndr: Very nicely done!

Answer 2

Extracting helper functions to add data to the return value means you can move the guard clauses to the helper functions, which seems to clarify the causes and effects happening here, and also will shorten the function signatures.

Untested code:

def parse(%HTTPCheckResult{parsed: false, http_response: {:ok, %Response{status_code: c}}} = check_result) do
 check_result
 |> add_parsed(true)
 |> add_code(c)
 |> handle_code(c)
end
def parse(%HTTPCheckResult{parsed: false, http_response: {:error, %HTTPoison.Error{reason: reason}}} = check_result) do
 check_result
 |> add_parsed(true)
 |> add_result(:error)
 |> add_error(reason)
end
def parse(%HTTPCheckResult{parsed: false} = result) do
 check_result
 |> add_result(:error)
 |> add_error("unknown")
end
defp add_parsed(result, val) do
 %{result | parsed: val}
end
defp add_code(result, val) do
 %{result | code: val}
end
defp add_result(result, val) do
 %{result | result: val}
end
defp add_error(result, val) do
 %{result | error: val}
end
defp handle_code(check_result, code) when code in @up do
 check_result
 |> add_result(:up)
end
defp handle_code(check_result, code) when code in @down do
 check_result
 |> add_result(:down)
end
defp handle_code(check_result, code) do
 check_result
 |> add_result(:error)
 |> add_error("http code #{code}")
end

• Scott Swezey: I really like this. I hadn't considered it at all. I may incorporate it into the refactor I just did.