Refactoring suggestions - error handling

Questions / Help
1

Hello, this code is currently working but I am having trouble figuring out how to make it clean.

The response is coming from an API.

Here is the code with included sample API response at the bottom: https://gist.github.com/tiotolstoy/1a0c0698f1128793d58386e70d9c9374

Many thanks.

2

Below you will find how I refactored your function.

Before you read that however, let me turn your attention to several mistakes that can potentially lead to bugs / runtime errors that you made and that have nothing to do with making your code made readable:

  1. You pattern match on {:error, _} and {:ok, map_with_certain_shape} (you assert that it has three particular keys which might not be the case). You should have a third clause which is either a catch-all – namely just _ – or it should be {:ok, map_without_the_keys_I_want_or_not_a_map}. :slight_smile:
  2. You again make that mistake below where you assert on result that can either have map_size or zero or just a map with some keys. OK, but what if it is not a map at all? You should put code in place that either throws an error or logs it.
  3. You introduce a bug by using an inconsistent coding style. On one place you check for an empty map with map_size(...) == 0 (which is the right way to do it) but then just match on %{} (not good, will match any map you pass to it!). Stick to the guard you used before or your second clause will never be executed.

Now, what I did change in your code:

  1. Moved the ok/error matches in their dedicated functions.
  2. Added one more ok clause which you didn’t cover (okay result with unexpected map shape, or just not a map at all).
  3. Replaced chained map fetches – map["a"]["b"]["c"] – with deep pattern-matching and moved those to a separate functions as well. (You again don’t do exhaustive pattern matching there.)

Here’s the modified code:

  def lookup_phone_number(params) do
    client().lookup_phone_number(params)
    |> lookup_phone_number_by_client_response()
  end

  defp lookup_phone_number_by_client_response({:error, _}) do
    %{
      carrier_type: nil,
      carrier_name: nil,
      country_code: nil,
      national_format: nil,
      caller_name: nil,
      caller_type: nil,
      mobile_country_code: nil,
      mobile_network_code: nil
    }
  end

  defp lookup_phone_number_by_client_response(
         {:ok,
          %{
            add_ons: add_ons,
            country_code: country_code,
            national_format: national_format
          }}
       ) do
    caller_info = caller_info(add_ons)
    carrier_info = carrier_info(add_ons)

    %{
      carrier_type: carrier_info.type,
      carrier_name: carrier_info.name,
      country_code: country_code,
      national_format: national_format,
      caller_name: caller_info.caller_name,
      caller_type: caller_info.caller_type,
      mobile_country_code: carrier_info.mobile_country_code,
      mobile_network_code: carrier_info.mobile_network_code
    }
  end

  defp lookup_phone_number_by_client_response({:ok, x}) do
    raise(
      ArgumentError,
      "Expected the OK result to contain a map with a certain shape but got this: #{inspect(x)}"
    )
  end

  def caller_info(%{
        "results" => %{"twilio_caller_name" => %{"result" => %{"caller_name" => caller}}}
      })
      when is_map(caller) and map_size(caller) == 0 do
    %{
      caller_name: nil,
      caller_type: nil
    }
  end

  def caller_info(%{
        "results" => %{"twilio_caller_name" => %{"result" => %{"caller_name" => caller}}}
      })
      when is_map(caller) do
    atomize_keys(caller)
  end

  def caller_info(x),
    do: raise(ArgumentError, "Unexpected input for fetching caller info: #{inspect(x)}")

  def carrier_info(%{
        "results" => %{"twilio_carrier_info" => %{"result" => %{"carrier" => carrier}}}
      })
      when is_map(carrier) and map_size(carrier) == 0 do
    %{
      mobile_country_code: nil,
      mobile_network_code: nil,
      name: nil,
      type: nil
    }
  end

  def carrier_info(%{
        "results" => %{"twilio_carrier_info" => %{"result" => %{"carrier" => carrier}}}
      })
      when is_map(carrier) do
    atomize_keys(carrier)
  end

  def carrier_info(x),
    do: raise(ArgumentError, "Unexpected input for fetching carrier info: #{inspect(x)}")

It’s a bit bigger than yours but it’s IMO more readable – and has more exhaustive pattern matching.

There’s also possibility for you to just make your own struct. Additionally, you repeat creating empty maps and copying values from maps a bit too much for my taste. You can just have a PhoneNumber struct which gets nil field values by default when created and just merge it with the data you get from the external API.