Using if/else

Questions / Help
1

The following code works:

  @impl true
  def handle_event("select-property", %{"property-id" => prop_id, "value" => "on"}, socket) do
    selected_properties = [String.to_integer(prop_id) | socket.assigns.selected_properties]

    {:noreply, assign(socket, selected_properties: Enum.uniq(selected_properties))}
  end

  @impl true
  def handle_event("select-property", %{"property-id" => prop_id}, socket) do
    prop_id = String.to_integer(prop_id)
    selected_properties = socket.assigns.selected_properties

    selected_properties = selected_properties
    |> Enum.reject(fn i -> i == prop_id end)
    |> Enum.uniq()
    {:noreply, assign(socket, selected_properties: selected_properties)}
  end

But the following code which is written using if/else doesn’t work. What I am doing wrong?

  @impl true
  def handle_event("select-property", %{"property-id" => prop_id, "value" => checked}, socket) do
    prop_id = String.to_integer(prop_id)

    if checked == "on" do
      selected_properties = [prop_id | socket.assigns.selected_properties]
    else
      selected_properties = socket.assigns.selected_properties
      |> Enum.reject(fn i -> i == prop_id end)
      |> Enum.uniq()
    end
    {:noreply, assign(socket, selected_properties: selected_properties)}
  end
2

In Elixir statements return the value, so you can’t do assignments inside a conditional (unless you’re using the assigned variable inside the same block).

This should work:

selected_properties =
  if checked == "on" do
    [prop_id | socket.assigns.selected_properties]
  else
    socket.assigns.selected_properties
    |> Enum.reject(fn i -> i == prop_id end)
    |> Enum.uniq()
  end
{:noreply, assign(socket, selected_properties: selected_properties)}
4 Likes
3

Thanks. That worked! In general is this a good practice? Or should I be using the function clauses approach?

4

It’s up to you! In this case, though, I would probably use different function clauses like in your first example. If I had more values to match on other than "on", and if this would result in a lot of duplicated code, I’d think of using case in a single function. But, again, there’s no universal rule.

5

An alternative refactoring:

  @impl true
  def handle_event("select-property", %{"property-id" => prop_id, "value" => checked}, socket) do
    prop_id = String.to_integer(prop_id)

    new_properties = update_selected(socket.assigns.selected_properties, prop_id, value)

    {:noreply, assign(socket, selected_properties: new_properties)}
  end

  def update_selected(properties, id, "on") do
    [id | properties]
  end

  def update_selected(properties, id, _) do
    properties
    |> Enum.reject(fn i -> i == id end)
    |> Enum.uniq()
  end

This separates the details of getting/setting socket.assigns.selected_properties from the details of maintaining the list of selected properties.

3 Likes
6

I like this alternative. It reads better.

Thank you!