Help refactor conditional block the Elixir way, please

josefrichter

Hi, I have this double condition if foo && bar do something and all the other cases basically don’t interest me. both foo and bar are assigns in a liveview socket, and I believe there’s a nicer, more elixir style way, of writing my function. I am thinking of some cond with pattern matching, but not sure how to put together the syntax - I am basically stuck at the very first line of the block :-o Can you please give me a hint? Thank you

    # 1. check whether we have socket.assign.user
    # 2. check if all todos are true
    IO.inspect socket.assigns
    socket =
      if (socket.assigns.user && list_completed?(socket.assigns.list_id)) do
        email = socket.assigns.user.email
        list_url = "https://organizer.gigalixirapp.com/#{list_id}"
        IO.puts "All tasks complete!"
        socket = 
          socket 
            |> put_flash(:info, "All tasks complete! Sending notification to #{email}...")
        Task.Supervisor.start_child(Organizer.TaskSupervisor, fn ->
          IO.puts "Sending to #{email} from within a Task"
          Organizer.Mailer.send_completion_notification(email, list_url)
        end)
        socket
      else
        IO.puts "Nope, either no user, or list not complete yet..."
        socket
      end 

    ...

    defp list_completed?(list_id) do
        Lists.list_todos(list_id)
            |> Enum.all?(&(&1.done)) # checks if all true https://hexdocs.pm/elixir/Enum.html#all?/2
    end

3 comments

/phoenix #pattern-matching #liveview #refactoring

0 1884 3

2020-06-11 20:41:38 UTC

Showing Posts 1 to 3

Show Best Posts
Show All (oldest first)
Show All (newest first)

gregvaughn

I wouldn’t refactor it. Yes, you could use a case, cond, or with, but I don’t think any of them bring clarity over what you have. You only have 2 cases and your conditions are boolean.

Post #1

al2o3cr

Here’s my take on it:

    IO.inspect socket.assigns

    if (socket.assigns.user && list_completed?(socket.assigns.list_id)) do
      email = socket.assigns.user.email
      list_id = socket.assigns.list_id

      send_completion_notification(email, list_id)

      IO.puts "All tasks complete!"

      put_flash(socket, :info, "All tasks complete! Sending notification to #{email}...")
    else
      IO.puts "Nope, either no user, or list not complete yet..."

      socket
    end
# ....
defp send_completion_notification(email, list_id) do
  Task.Supervisor.start_child(Organizer.TaskSupervisor, fn ->
    IO.puts "Sending to #{email} from within a Task"
    list_url = "https://organizer.gigalixirapp.com/#{list_id}"
    Organizer.Mailer.send_completion_notification(email, list_url)
  end)
end

If possible, I like to avoid rebinding (the socket = ...something that uses socket... pattern) partly because it adds indentation and partly because it’s a warning sign of complexity.

Sometimes you can avoid that by shuffling operations; this version starts the task before calling put_flash (versus the original that does the reverse) but there shouldn’t be any observable side-effect.

I broke send_completion_notification out into a private function to keep the main chunk of code focused at a single level of abstraction: manipulating socket’s contents.

Bigger tidying that could be done, depending on the context: is there something that ensures socket.assigns.user is set before this code runs? If so, consider skipping the re-check in the if clause and Let It Crash.

Post #2