Using try/catch with update_in/3

I’m working on a small exercise involving update_in/3, and I came up with this solution:

data = %{
  name: "Periodic Table",
  category: "Chemistry",
  elements: %{
    hydrogen: ["H", "Nonmetal"],
    helium: ["He", "Noble gas"],
    lithium: ["Li"],
    oxygen: ["O", "Nonmetal"]
  },
  version: 1
}

remove_attribute = fn data, element, attribute ->
  try do
    update_in(
      data,
      [:elements, element],
      fn
        nil -> throw(:element_not_found)
        [^attribute] -> throw(:remove_element)
        [_|_] = attributes -> List.delete(attributes, attribute)
      end
    )
  catch
    :element_not_found -> data
    :remove_element -> %{data | elements: Map.delete(data.elements, element)}
  end
end

The behavior I want is:

  • If the element has several attributes, remove the given attribute from the list.

  • If the attribute being removed is the element’s last attribute, remove the element itself from the :elements map.

  • If the element or attribute doesn’t exist, leave the data unchanged.

Is this a valid use of try/catch and throw? How would you approach this?

Have you consider using get_and_update_in/3 instead (returning :pop when element removal is required)?

No. See Design-related anti-patterns — Elixir v1.20.4

Something like this?

  def remove_attribute(%{elements: elements} = data, element, attribute) do
    elements = elements
               |> update_in([element], &(List.wrap(&1) -- [attribute]))
               |> Enum.reject(fn {_ , value} -> match?([], value) end)
               |> Map.new

    %{data | elements: elements}
  end

get_and_update_in is the solution here with :pop, but also using Access.key with a default of [] removes the need to special case the non existing key path:

{_, result} =
  get_and_update_in(data, [:elements, Access.key(:dragon, [])], fn attributes ->
    case List.delete(attributes, attribute) do
      [] -> :pop
      attributes -> {nil, attributes}
    end
  end)

The attributes case should return {nil, attributes}.

I think that anti-pattern refers to try/rescue, not try/catch.
I agree that try/catch is ugly and I try to avoid it as much as I can.

If I were you, I will just implement the algorithm as closely as possible to your 3 bullet points. I believe someone said: “Don’t try to be as smart as possible in writing the code, because then you’d need to be smarter still the debug it”

new_elements = 
    data.elements
    |> Enum.map(fn {name, attributes} -> {name, List.delete(attributes, attribute)} end)
    |> Enum.filter(fn {name, attributes} -> attributes != [] end)
    |> Map.new()

data = %{data | elements: new_elements} 

PS: The source of the quote is Kernighan’s law

That one would remove the attribute from any element not just the specified one.

Oops, that’ll be:

case Map.fetch(data.elements, element) do
    {:ok, attributes} ->
        case List.delete(attributes, attribute) do
            [] -> pop_in(data, [:elements, element])
            attributes -> put_in(data, [:elements, element], attributes)
        end

    :error ->
        data
end

So my solution would satisfy every requirement including Kernighan’s law :slight_smile:

Funny also how devs prefer different styles.
Whenever I write case I’ll always think “stop right there” why are you not using the pipe operator.
If @LostKobrakai were in my team, I would tell him to stick to the assignment and please write readable code :stuck_out_tongue:

I default to case statements because they match my mental model and are very expressive. Only when I have 3+ levels of cascading case statements and they become unwieldy, then I start thinking about refactoring into with or piping through private functions.

Yes case statements are very expressive, fully agree, (but) so are function heads.
I tend to better describe functions than cases even just naming a function feels like describing, while case oftentimes “hides” a moment of clarity, like later on you think huh!?

Anyway, I think it’s a good thing preferences exist.