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

Questions / Help
1

It was a much simpler exercise, but I wanted to add a few things, and well, aside from the duplication, it’s ugly.

defmodule JsonAPI do

  def query(cat,id,keys) do

    categories = %{
      "posts"    => 100,
      "comments" => 500,
      "albums"   => 100,
      "photos"   => 5000,
      "todos"    => 200,
      "users"    => 10
    }

    if cat not in Map.keys(categories) do
       {:error, ~s('#{cat}' is not a valid category.) }
     else
       if id > categories[cat] do
         {:error, ~s(The maximum id for '#{cat}' is #{categories[cat]}.) }
       else
         base = "https://jsonplaceholder.typicode.com/"
         [base, cat, "/", to_string(id)]
               |> :erlang.iolist_to_binary
               |> HTTPoison.get
               |> handle_response(keys)
       end
     end
   end

   def query(cat,id) do

     categories = %{
       "posts"    => 100,
       "comments" => 500,
       "albums"   => 100,
       "photos"   => 5000,
       "todos"     => 200,
       "users"    => 10
     }

     if cat not in Map.keys(categories) do
       {:error, ~s('#{cat}' is not a valid category.) }
     else
       if id > categories[cat] do
         {:error, ~s(The maximum id for '#{cat}' is #{categories[cat]}.) }
       else
         base = "https://jsonplaceholder.typicode.com/"
         [base, cat, "/", to_string(id)]
               |> :erlang.iolist_to_binary
               |> HTTPoison.get
               |> handle_response
       end
     end
   end

   def query(cat) do

     categories = %{
       "posts"    => 100,
       "comments" => 500,
       "albums"   => 100,
       "photos"   => 5000,
       "todos"     => 200,
       "users"    => 10
     }

     if cat not in Map.keys(categories) do
       {:error, ~s('#{cat}' is not a valid category.) }
     else
         base = "https://jsonplaceholder.typicode.com/"
         [base, cat]
               |> :erlang.iolist_to_binary
               |> HTTPoison.get
               |> handle_response
     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
 end
1 Like
2

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.

3 Likes
3

You can convert the nested if/else to functions.

2 Likes
4

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

1 Like
5

@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
1 Like
6

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
7

which course is this from?

8

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.

9

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.

10 
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!

11

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…

1 Like
12

@lgp :+1:

  • keys and id logic is being spread all over utility functions like get_url and handle_response.

Check if this works:

defmodule JsonApi do
  def query(cat, id \\ 0, keys \\ []) do
    categories = %{
      "posts"    => 100,
      "comments" => 500,
      "albums"   => 100,
      "photos"   => 5000,
      "todos"    => 200,
      "users"    => 10
    }
    base = "https://jsonplaceholder.typicode.com/"
    
    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]}.)}

      is_list(keys) and length(keys) > 0 ->
        url = [base, cat, "/", to_string(id)]
        response = get_data(url)
        case response do
          {:ok, target} ->
            get_in(target, keys)

          _ ->
            response 
        end

      id > 0 ->
        url = [base, cat, "/", to_string(id)]
        get_data(url)

      true ->
        url = [base, cat]
        get_data(url)
    end
  end

  defp handle_response({:ok, %{status_code: 200, body: body}}) do
    target = Poison.Parser.parse!(body, %{})
    {:ok, target}
  end

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

    {:error, status, message}
  end

  defp handle_response({:error, reason}), do: {:error, reason}

  defp get_data(url) do
    url
    |> :erlang.iolist_to_binary()
    |> HTTPoison.get()
    |> handle_response()
  end
end
1 Like
13

Much nicer! Thanks!

  • Larry
1 Like
14

Personally, I like to paraphrase Thomas Jefferson with “One codes best who codes least”.
While I highly admire and recommend the excellent courses from Pragmatic Studios, I often wince when coming across code that is unhelpfully verbose or not refactored before put into production. With a functional language, there is seldom any advantage to the use of if-else or cond clauses IMHO.
Here’s my take:

defmodule JsonAPI do
    @cats %{"posts" => 100,"comments" => 500,"albums" => 100,"photos" => 5000,"todos" => 200,"users" => 10}
    @base "https://jsonplaceholder.typicode.com/"
    
    def query(cat, id, keys). do: check_key(cat) |> chk_range(cat, id) |> re_query(cat, id, keys) 
    def query(cat, id),       do: check_key(cat) |> chk_range(cat, id) |> re_query(cat, id)
    def query(cat),           do: check_key(cat)                       |> re_query(cat)
    
    def check_key(cat),                                      do: check_key(cat, Map.keys(@cats))
    def check_key(cat, cat_keys) when cat in cat_keys,       do: :ok
    def check_key(cat, _ ),                                  do: {:error, "'#{cat}' is not a valid category." }  
    
    def chk_range(:ok, cat, id)  when id <= @cats(cat),      do: :ok
    def chk_range(:ok, cat, _ ),                             do: {:error, "The maximum id for '#{cat}' is #{@cats[cat]}." }
    def chk_range(error, _, _ ),                             do: error
      
    def re_query(:ok, {cat, id, keys}),                      do: lookup(cat, id) |> respond(keys)
    def re_query(:ok, {cat, id      }),                      do: lookup(cat, id) |> respond
    def re_query(:ok, {cat          }),                      do: lookup(cat)     |> respond
    def re_query({:error, msg},  _   ),                      do: msg 
  
    def lookup(cat, id),                                     do: HTTPoison("#{@base}#{cat}/#{to_string(id)}")
    def lookup(cat)    ,                                     do: HTTPoison("#{@base}#{cat}")
    
    def respond( {:ok, %{status_code:  200,   body: b}}, k), do: {:ok,            get_in(Poison.Parser(b, %{}),  k      )}
    def respond( {:ok, %{status_code: status, body: b}}, _), do: {:error, status, get_in(Poison.Parser(b, %{}), ["message"])}
    def respond( {:error, reason }, _ ),                     do: {:error, reason}

    end
...
2 Likes
15

Did you try to run it? I had a few problems…

16

Thank you for providing a pattern matching version of the code.

Pattern matching provides a way to write code without if, cond or case - but there is no reason to avoid them completely. Elixir lang repo contains many instances of if, cond and case along with pattern matching.

17

No, in fact I noticed some typos after I sent it. It was a “quick and dirty” but I’ll continue on it if you want more help.

18

Agreed. When I am first developing a module, I often use some conditionals for expediency but almost always find that my code is cleaner and clearer after refactoring later.

1 Like
19

It should be noted that I am well aware that my coding style will be considered my many to be heretical. I come from the Erlang camp and have never been comfortable with OO or procedural languages.

1 Like
20

Yeah, I fixed the typos (the missing function names and the period instead of a comma), but I couldn’t figure out how to fix this:
def chk_range(:ok, cat, id) when id <= @cats(cat),

I assumed that the parens should be brackets, but that led to a:
cannot invoke remote function Access.get/2 inside guards

And that’s where I gave up on it.