lgp

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

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

23 comments

#exercises

19 1599 23

2022-03-10 20:14:08 UTC

Marked As Solved

kartheek

@lgp

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

Post #12

Also Liked

kartheek

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.

Post #2

kodepett

You can convert the nested if/else to functions.

Post #3

drolll

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

Post #14

drolll

One more cleanup tweak:

defmodule JsonAPI do
  @c %{"posts"=>100,"comments"=>500,"albums"=>100,"photos"=>5000,"todos"=>200,"users"=>10}
  @b "https://jsonplaceholder.typicode.com/"

  def query(c, id \\ 0, keys \\ []), do: chk_input(c, id) |> request({c, id, keys})

  defp chk_input(c, id),             do: is_cat(c in Map.keys(@c), c, id)

  defp is_cat(true, c, id),          do: in_range(id <= @c[c], c)
  defp is_cat(_,    c, _ ),          do: {:error, "'#{c}' is invalid"}
  defp in_range(true, _ ),           do: :ok
  defp in_range( _  , c ),           do: {:error, "#{c} max id: #{@c[c]}"}

  defp request( :ok, {c, id, keys}), do: lookup(c, id) |> reply(keys)
  defp request({:error, msg},  _  ), do: msg

  defp lookup(c, 0),                 do: HTTPoison.get("#{@b}#{c}")
  defp lookup(c, i),                 do: HTTPoison.get("#{@b}#{c}/#{to_string(i)}")

  defp reply({:error, reason }, _ ),              do: {:error,  reason}
  defp reply({_,%{status_code: 200,body: b}},[]), do: {:ok,     parse(b) }
  defp reply({_,%{status_code: 200,body: b}},k),  do: {:ok,     filter(b, k) }
  defp reply({_,%{status_code:  s, body: b}},_),  do: {:error,s,filter(b,["message"])}

  defp filter(body, keys),           do: parse(body) |> get_in(keys)
  defp parse(body),                  do: Poison.Parser.parse!(body, %{})
  end

Post #23