HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Pattern matching deeply nested arguments in Elixir

Submitted by: @import:stackexchange-codereview··
0
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 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?

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

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

Context

StackExchange Code Review Q#131502, answer score: 3

Revisions (0)

No revisions yet.