rabet

Learning / proper use of cond (and other Elixir style)

Hey all dear Elixir fans and pros,

I try to learn with wiritng the most-elixirish functional code, sometime with the help of you.

I am just writing a parsing module, which extracts some info from recieved JSON (different shapes), and come up with this functions:

def get_markets do
    for {market, url} <- get_markets_urls() do # get_markets_urls just query my db for market name and it's url (about 5-10 records max.)
      case get(url) do
        {:ok, val} -> parse_markets(val.body, market)
        {:error, reason} -> IO.puts "Error: #{reason}"
      end
    end
  end

  defp parse_markets(response, market) do
    IO.puts "Parsing markets"
    cond do
      market == "Market1" ->
        IO.puts Enum.map(response, &(Map.get(&1, "asset")))
      market == "Market2" ->
        Enum.map(Map.get(response, "data"), &(marketClient(&1, market)))
      market == "Market3" ->
        IO.puts Enum.map(response, &(&1))
      true -> "Other"
    end
  end

As you can see, for every market I need to extract different info (because of different response shape).
Please, is this a good way to write it? Coud it be written by better, more functional way?

Thanks for the hints!

4 comments

#code-review #idiomatic-elixir #cond

0 2408 4

2019-02-15 10:37:22 UTC

Most Liked

kokolegorille

Maybe You can improve with multiple functions… like this

defp parse_markets(response, "Market1"), do: #whatever
defp parse_markets(response, "Market2"), do: #whatever
defp parse_markets(response, "Market3"), do: #whatever
defp parse_markets(response, _), do: #whatever

Post #2

peerreynders

Also while there is nothing wrong with comprehensions it seems that your are holding on too tightly to iteration.

If you are going through the list for side effects I’d expect to see Enum.each/2 instead or if you wanted conditional transformation I’d expect some piped combination of Enum.filter/2 and Enum.map/2 (though comprehensions can have filters as well).

At the very least I’d make the comprehension look less like a loop, e.g.

def get_markets, 
  do: for {name, url} <- get_market_urls()
          do: get_market_data(name, url)

def get_markets,
  do: Enum.map(get_market_urls(), fn {name,url} -> get_market_data(name,url) end)

def get_market_data({name,url}) do: 
  ...
end

def get_markets,
  do: Enum.map(get_market_urls(), &get_market_data/1)

(In general I’d dial back on the use of inline anonymous functions that is so common in JavaScript)

Post #4

NobbZ

As all your branches do compare for equality of market against a string literal a case were much more idiomatic.

Also I’m not quite sure why in the parse_markets/2 you are returning :ok (an atom) most of the time but "Other" (a string) on unknown markets.

Post #3

Last Post!

iboard

A “Clean Coder” advice: I would prefer the pattern-match solution by @kokolegorille for one reason: You can add new “Markets” without changing the existing code but by just adding a new function-header for “Market4”.

Post #5