Help me make this more idiomatic

Questions / Help
1

Hi all - been learning Elixir the last few weeks and loving it.

I have a simple shopping cart with the following structure in an Agent: %{"cart_id" => %{"item1" => 1, "item2" => 2}. How can I make the following code more concise/idiomatic?

  # Data shape:
  # %{"cart_id" => %{"item1" => 1, "item2" => 2}

  def increment(cart_id, item_id) do
    Agent.update(__MODULE__, fn state ->
      Map.update(state, String.to_atom(cart_id), %{item_id => 1}, fn cart ->
        if Map.get(cart, item_id) do
          Map.update(cart, item_id, 1, &(&1 + 1))
        else
          cart
        end
      end)
    end)
  end

  def decrement(cart_id, item_id) do
    Agent.update(__MODULE__, fn state ->
      Map.update(state, String.to_atom(cart_id), %{item_id => 1}, fn cart ->
        if Map.get(cart, item_id) do
          Map.update(cart, item_id, 1, fn v ->
            if v > 1 do
              v - 1
            else
              v
            end
          end)
        else
          cart
        end
      end)
    end)
  end
1 Like
2

I think more idiomatic way to do so is refactor to GenServer. Make some private functions on server side. It would look much better. Your logic have ‘grown’ over Agent functionality.

1 Like
3

This is not recommended, because atoms are not garbage collected. You might end up reaching the max atoms limit.

I also try to avoid using if whenever I can, and nested if too, I prefer to use with in that case.

5 Likes
4

Access and Kernel.update_in/3 are your friends. Somewhat like

update_in(state, [cart_id, item_id], fn
  nil -> 1
  int when is_integer(int) -> int + 1
end)
4 Likes
6

Thanks all for the help. I’ve got something now which looks a good bit better.

def increment_item(cart_id, item_id) do
  Agent.update(__MODULE__, fn state ->
    case state[cart_id][item_id] do
      nil ->
        state
      _ ->
        update_in(state, [cart_id, item_id], fn
          int when is_integer(int) -> int + 1
          _ -> 1
        end)
    end
  end)
end

def decrement_item(cart_id, item_id) do
  Agent.update(__MODULE__, fn state ->
    case state[cart_id][item_id] do
      nil ->
        state
      _ ->
        update_in(state, [cart_id, item_id], fn
          int when is_integer(int) and int > 1 -> int - 1
          _ -> 1
        end)
    end
  end)
end
7

I tried to implement this as a small exercise. I am not sure if it is perfectly idiomatic, but I did my best. Feedback is welcome, of course.

I put most of the logic into a plain (“pure”) module, so it’s easily testable and used Agent only for the necessary (stateful) stuff.

I am not sure if it is desired behavior, but in your solution, if I try to increment the count of a non-present item, it will do nothing. In my solution, it will add it into the cart and set its count to 1. Also, in your solution, you’re not able to decrement the count from 1 to 0. I would assume that decrementing count from 1 would set it to 0 or remove it from the cart. But those are business logic details, that are easily adjustable.

Also, in my opinion, having (doc)tests for a seemingly simple algorithm like this is always helpful, not only to refactor it with confidence but also to serve as documentation. I don’t want to be captain obvious here, but since you asked for an idiomatic solution, I put it there.

defmodule Carts do
  @doc """
  ## Examples:

      # Incrementing item count in non-existing cart
      # will do nothing and return the original carts
      iex> Carts.increment(%{}, "cart_id", "item_id")
      %{}

      # Incrementing count of non-existing item
      # will add it to a cart and set its count to 1
      iex> Carts.increment(%{"cart_id" => %{}}, "cart_id", "item_id")
      %{"cart_id" => %{"item_id" => 1}}

      # Standard scenario
      iex> Carts.increment(%{"cart_id" => %{"item_id" => 3}}, "cart_id", "item_id")
      %{"cart_id" => %{"item_id" => 4}}
  """
  def increment(carts, cart_id, item_id)
      when is_map(carts) and is_binary(cart_id) and is_binary(item_id) do
    case carts[cart_id] do
      nil ->
        carts

      _cart ->
        update_in(carts, [cart_id, item_id], fn
          nil -> 1
          count -> count + 1
        end)
    end
  end

  @doc """
  ## Examples:

      # Decrementing item count in non-existing cart
      # will do nothing and return the original carts
      iex> Carts.decrement(%{}, "cart_id", "item_id")
      %{}

      # Decrementing count of non-existing item
      # will do nothing and return the original carts
      iex> Carts.decrement(%{"cart_id" => %{}}, "cart_id", "item_id")
      %{"cart_id" => %{}}

      # Decrementing count of existing item with count 0
      # will do nothing and return the original carts
      iex> Carts.decrement(%{"cart_id" => %{"item_id" => 0}}, "cart_id", "item_id")
      %{"cart_id" => %{"item_id" => 0}}

      # Standard scenario
      iex> Carts.decrement(%{"cart_id" => %{"item_id" => 3}}, "cart_id", "item_id")
      %{"cart_id" => %{"item_id" => 2}}
  """
  def decrement(carts, cart_id, item_id)
      when is_map(carts) and is_binary(cart_id) and is_binary(item_id) do
    case carts[cart_id][item_id] do
      nil -> carts
      0 -> carts
      _count -> update_in(carts, [cart_id, item_id], &(&1 - 1))
    end
  end
end

defmodule CartsAgent do
  def increment(cart_id, item_id) do
    Agent.update(__MODULE__, fn carts ->
      Carts.increment(carts, cart_id, item_id)
    end)
  end

  def decrement(cart_id, item_id) do
    Agent.update(__MODULE__, fn carts ->
      Carts.decrement(carts, cart_id, item_id)
    end)
  end
end
4 Likes