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?

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.

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