Better way to write this?

Questions / Help
1 
def raw_heroes(playertag) do
    with {:ok, account} <- Raw.get_raw_player_information(playertag) do
      Map.take(account, ["heroes"])
      |> Map.values()
      |> hd()
    end
  end

  def find_by_name(playertag, heroname) do
    raw_heroes(playertag)
    |> Enum.find(fn(hero) -> hero["name"] == heroname end)
  end

  def get_level_of_barbarian_king(playertag) do
    find_by_name(playertag, "Barbarian King")["level"]
  end

  def get_level_of_archer_queen(playertag) do
    find_by_name(playertag, "Archer Queen")["level"]
  end

I know this code is ugly, it works but its hurting my eyes with Map etc. any change I can replace that with something else?

2

Someone wanting to help may be interested in reading more context for this question from the Elixir Discord, starting from this message (you need to have joined the server already for the link to work properly, I believe): Discord

3

This has identical behavior but is shorter:

Map.get_lazy(account, "heroes", fn -> hd([]) end)

or if the behavior when account doesn’t have a key "heroes" isn’t important:

account["heroes"]
4

Really depends if you e.g. care about error handling and want stuff to crash when the shape of the data is not what you expect.

Example:

This will crash if "heroes" key contains nil or it doesn’t even exist in the map. Or if it returns an empty list. hd requires a non-empty list and raises an error if not.

If you’re okay with having nil or [] flying around then change the code to:

      Map.take(account, ["heroes"])
      |> Map.values()
      |> List.wrap()  # converts `nil` to `[]`
      |> List.first() # returns `nil` if the list is empty

Another thing – a very controversial one in the Elixir community I think – is pipe consistency. Personally I would not write this:

I would change it to:

    playertag
    |> raw_heroes()
    |> Enum.find(fn(hero) -> hero["name"] == heroname end)
3 Likes
5

This might as well be something like: {:ok, %{"heroes" => heroes}} = Raw.get_raw_player_information(tag), not sure about the data structure, but you said it works, so assuming heroes is a required field for Player, you might not need a with here, as both version will raise errors here if "heroes" is missing from the structure. So the final code might look something like:

def raw_heroes(playertag) do
  {:ok, %{"heroes" => heroes}} = Raw.get_raw_player_information(playertag)
  heroes
end

In case you want to deal with absence of key and avoid the MatchError you could make it a with.

The level getters look good, but without seeing the bigger picture, not sure I can suggest any alternative (i.e. how I’d do it if I were to make it?).

1 Like
6

Here you go:

  def raw_heroes(playertag) do
    case Raw.get_raw_player_information(playertag) do
      {:ok, %{"heroes" => heroes}} when is_list(heroes) -> heroes
      _ -> []
    end
  end

  def find_by_name(playertag, heroname) do
    playertag |> raw_heroes() |> Enum.find(&(&1["name"] == heroname))
  end

  def get_level_of_archer_queen(playertag) do
    find_by_name(playertag, "Archer Queen")["level"]
  end

  def get_level_of_barbarian_king(playertag) do
    find_by_name(playertag, "Barbarian King")["level"]
  end

My few cents here is a case statement. Using it like I did above gives you a list no matter what happens i.e. when:

  1. it’s not {:ok, result} tuple
  2. map does not contain "heroes" key
  3. the value of "heroes" key is not a list (for example nil)

the function returns empty list [] for those cases.

3 Likes