First Elixir Project (Probably terrible code)

Questions & Help Questions
, ,
10

Some other general feedback (not Elixir specific, hope that’s ok):

Improve names

  • Method names like make_request can feel obvious in the moment you are writing the code, but don’t truly communicate what they are doing. Instead, consider lookup_by_domain.

Do one thing

  • Most of the get_ methods are doing more than one thing, for example:
 def get_subdomains(builtwithjson) do
      get_results(builtwithjson)
      |> Enum.find(fn map -> Map.has_key?(map, "Result") end)
      |> case do
        nil ->
          nil
        subdomains ->
        Map.get(subdomains, "Result")
        |> Map.get("Paths")
        |> Enum.map(fn x -> Map.get(x, "SubDomain") end)
        |> Enum.reject(&(&1 == ""))
      end
  end

You are getting the first result from a list of results and getting the SubDomain from it. I know most likely the first result of the list is probably the correct one, otherwise you wouldn’t have built it this way. The issue though is that your library is making a choice that’s hidden from the user. Namely, which one of the results is “correct.” What if my lookup by domain had multiple results and I actually wanted the second one? Instead, make these get_ methods only work with a Result, not a list of Results, e.g. change the above to:

 def get_subdomains(builtwithjson_result) do
      builtwithjson_result
          |> Map.get(subdomains, "Result")
          |> Map.get("Paths")
          |> Enum.map(fn x -> Map.get(x, "SubDomain") end)
        |> Enum.reject(&(&1 == ""))
  end

Now, you do force the library user to pick which result they want, and they could get an error when using the get_ methods, but your current library has the following behavior:

# Imagine the list of results has, in order:
# 1. A result with a "Result" field, but no "Attributes" field.
# 2. A result with an "Atttribute" field.
results = Builtwith.lookup_by_domain("google.com", my_api_key)

# Reads the "SubDomain" field from the first entry in the list of results. 
subdomains = results |> Builtwith.get_subdomains() 

# Read the Attributes field from the second entry in the list of results.
attributes = results |> Builtwith.get_attributes()

The user now has a list of subdomains and a list of attributes, but they aren’t from the same result. This could be very bad for some APIs. This exact scenario may be impossible with the Builtwith API, but it could be possible with another API with a different data model and could have very bad consequences if you get this mixed up.

Micro optimization: Avoid double Enum calls

This is a small one and someone may correct me if I’m incorrect about this. In get_subdomains:

|> Enum.map(fn x -> Map.get(x, "SubDomain") end)
|> Enum.reject(&(&1 == ""))

Each call to Enum will basically allocate a new list (which will increase memory usage). Instead you can use the Stream API:

|> Stream.map(fn x -> Map.get(x, "SubDomain") end)
|> Enum.reject(&(&1 == ""))

This will lead to a slight optimization to the memory footprint of your code. Note, Streams are lazy - the output of a Stream method is always a Stream, so you will always need to end with an Enum method to get the type you actually want at the end.