6
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ @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. \$\endgroup\$ – Scott Swezey Jun 10 '16 at 5:43
  • \$\begingroup\$ 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. \$\endgroup\$ – 200_success Jun 10 '16 at 6:37
  • \$\begingroup\$ 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. \$\endgroup\$ – Scott Swezey Jun 10 '16 at 8:19
  • \$\begingroup\$ 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. \$\endgroup\$ – Mathieu Guindon Jun 10 '16 at 13:31
3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Very nicely done! \$\endgroup\$ – alxndr Jun 12 '16 at 15:28
2
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ I really like this. I hadn't considered it at all. I may incorporate it into the refactor I just did. \$\endgroup\$ – Scott Swezey Jun 11 '16 at 22:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.