How do I clean up this code from an example in the PragProg elixir course?

Hi @lgp can you add new line with ``` at the beginning of the code block and end of code blocks. You can edit post and add them:

```
Your Code
```

Also your question is not clear as to what you want to cleanup ? Can you provide more details.

You can convert the nested if/else to functions.

OK. How do I edit the post? I haven’t found a way.

@lgp IIRC there’s an edit timeout for regular users or something, I went ahead and added backticks to your post.

Some general thoughts on cleaning up the code:

• chains of if / else may be more readable as cond
• a common idiom with functions with optional arguments is that the shorter versions (like query/1 and query/2 above) fill in the remaining arguments and call the “longer” version (query/3 here)
• consider extracting common stanzas (like the lines that start with base = "https://jsonplaceholder.typicode.com/") to a private function instead of repeating them

Thanks very much. I got rid of the nested ifs.

There really are no default arguments for the query/1 and query/2 versions. I would have to put so many conditionals inside the main function that I would be no better off. I’ll keep looking at that, though.

I’ll also look at using a private function for the URL building lines. Not sure how much that would save since handling the various options would introduce a lot of complications I think.

Again, thanks for the reply – and for adding the back ticks!

• Larry

which course is this from?

The Pragmatic Studio “Elixir/OTP” course. This exercise was from the notes, not one of the videos. And as I mentioned, I expanded on it a bit.

OK. Took care of the other suggestions.
larry@habu lib % ll json*
-rw-r–r–@ 1 larry staff 2787 Mar 5 17:57 json_api.ex
-rw-r–r–@ 1 larry staff 2003 Mar 5 17:57 json_api.new.ex
larry@habu lib % wc json*
112 301 2787 json_api.ex
90 213 2003 json_api.new.ex
202 514 4790 total
larry@habu lib %
I’ll post the new version for comparison.

defmodule JsonAPI do
  def query(cat, id \\ 0, keys \\ []) do
    categories = %{
      "posts"    => 100,
      "comments" => 500,
      "albums"   => 100,
      "photos"   => 5000,
      "todos"    => 200,
      "users"    => 10
    }

    cond do
      cat not in Map.keys(categories) ->
        {:error, ~s('#{cat}' is not a valid category.)}

      id > categories[cat] ->
        {:error, ~s(The maximum id for '#{cat}' is #{categories[cat]}.)}

      true ->
        get_url(cat, id, keys)
    end
  end

  def handle_response({:ok, %{status_code: 200, body: body} = _response}, keys) do
    target =
      body
      |> Poison.Parser.parse!(%{})
      |> get_in(keys)

    {:ok, target}
  end

  def handle_response({:ok, %{status_code: status, body: body} = _response}, _keys) do
    message =
      body
      |> Poison.Parser.parse!(%{})
      |> get_in(["message"])

    {:error, status, message}
  end

  def handle_response({:error, reason}, _) do
    {:error, reason}
  end

  def handle_response({:ok, %{status_code: 200, body: body} = _response}) do
    target =
      body
      |> Poison.Parser.parse!(%{})

    {:ok, target}
  end

  def handle_response({:ok, %{status_code: status, body: body} = _response}) do
    message =
      body
      |> Poison.Parser.parse!(%{})
      |> get_in(["message"])

    {:error, status, message}
  end

  def handle_response({:error, reason}) do
    {:error, reason}
  end

  defp get_url(cat, id, keys) do
    base = "https://jsonplaceholder.typicode.com/"

    cond do
      is_list(keys) and length(keys) > 0 ->
        [base, cat, "/", to_string(id)]
        |> :erlang.iolist_to_binary()
        |> HTTPoison.get()
        |> handle_response(keys)

      id > 0 ->
        [base, cat, "/", to_string(id)]
        |> :erlang.iolist_to_binary()
        |> HTTPoison.get()
        |> handle_response

      true ->
        [base, cat]
        |> :erlang.iolist_to_binary()
        |> HTTPoison.get()
        |> handle_response
    end
  end
end

Still need to work on the duplication in handle_response, but this is a great improvement. Thanks again!

And now cleaned up handle_response. Overall quite an improvement:
larry@habu lib % wc json*
113 304 2873 json_api.ex
73 185 1669 json_api.fin.ex
186 489 4542 total
larry@lil-habu lib %

And the (for now) final version:

defmodule JsonAPI do
  def query(cat, id \\ 0, keys \\ []) do
    categories = %{
      "posts"    => 100,
      "comments" => 500,
      "albums"   => 100,
      "photos"   => 5000,
      "todos"    => 200,
      "users"    => 10
    }

    cond do
      cat not in Map.keys(categories) ->
        {:error, ~s('#{cat}' is not a valid category.)}

      id > categories[cat] ->
        {:error, ~s(The maximum id for '#{cat}' is #{categories[cat]}.)}

      true ->
        get_url(cat, id, keys)
    end
  end

  def handle_response( response, keys \\ [])

  def handle_response({:ok, %{status_code: 200, body: body} = _response}, keys) do
    target =
      body
      |> Poison.Parser.parse!(%{})
    if length(keys) > 0 do
      {:ok, target |> get_in(keys) }
    else
      {:ok, target}
    end
  end

  def handle_response({:ok, %{status_code: status, body: body} = _response}, _keys) do
    message =
      body
      |> Poison.Parser.parse!(%{})
      |> get_in(["message"])

    {:error, status, message}
  end

  def handle_response({:error, reason}, _) do
    {:error, reason}
  end

  defp get_url(cat, id, keys) do
    base = "https://jsonplaceholder.typicode.com/"

    cond do
      is_list(keys) and length(keys) > 0 ->
        [base, cat, "/", to_string(id)]
        |> :erlang.iolist_to_binary()
        |> HTTPoison.get()
        |> handle_response(keys)

      id > 0 ->
        [base, cat, "/", to_string(id)]
        |> :erlang.iolist_to_binary()
        |> HTTPoison.get()
        |> handle_response

      true ->
        [base, cat]
        |> :erlang.iolist_to_binary()
        |> HTTPoison.get()
        |> handle_response
    end
  end
end

Thanks once more…