Let's refactor this code with multiple case clauses

Questions / Help
1

Hi!

I often end up with multiple, nested case clauses and return values. I’m not really sure on how to split these up in a nice way to make it easier to read and understand/reason about.

I’ve read that the with keyword is often used to avoid nested cases, but I don’t really see how it would help me here.

So, why not refactor this code snippet together and learn something :smiley: I think it can lead to some valuable discussions with an actual code example.

How would you do it? Split into functions? Separate modules? Leave it as is?

Scenario:
I have a controller action which receives a push token and a service (gcm/apns) and it should register this to Amazon SNS.

  1. Receive token and service.
  2. Register token at Amazon SNS (implementation of that is left out for brevity) and get an endpoint arn.
    2.a. If it errors, return errors.
    2.b. Success! Continue.
  3. Check if the endpoint arn is already present in our DB.
    3.a. Present? Do nothing and return 201.
    3.b. Not present? Continue below.
  4. Save the endoint arn in the DB.
    4.a. Errors? Return errors.
    4.b. Success! Return 201.

Code:

def create(conn, %{"push_token" => %{"service" => service, "token" => token}}) do
  case MyApp.Push.Sns.register_token(token, service) do
    {:ok, endpoint_arn} ->
      case Repo.get_by(PushToken, endpoint_arn: endpoint_arn) do
        nil -> # Token does not exist, store it
          changeset =
            conn.assigns.current_user
            |> build_assoc(:push_tokens)
            |> PushToken.changeset(%{endpoint_arn: endpoint_arn})

          case Repo.insert(changeset) do
            {:ok, _} ->
              conn
              |> send_resp(201, "")
            {:error, changeset} ->
              conn
              |> put_status(:unprocessable_entity)
              |> render(MyApp.ChangesetView, "error.json", changeset: changeset)
          end
        _ -> # Token exists do nothing
          conn
          |> send_resp(201, "")
      end
    {:error, message} ->
      conn
      |> put_status(:bad_request)
      |> render(MyApp.ErrorView, "400.json", message: message)
  end
end
3 Likes
2

I can’t adivse that talk too much. TL;DR : Use functions

5 Likes
3

Untested, but you can do something like:

def create(conn, %{"push_token" => %{"service" => service, "token" => token}}) do
  case create_and_register_token(conn.assigns.current_user, token, service) do
    {:ok, _} ->
      send_resp(conn, 201, "")
    {:error, %Ecto.Changeset{} = changeset} ->
      conn
      |> put_status(:unprocessable_entity)
      |> render(MyApp.ChangesetView, "error.json", changeset: changeset)
    {:error, message} ->
      conn
      |> put_status(:bad_request)
      |> render(MyApp.ErrorView, "400.json", message: message)
  end
end


def create_and_register_token(user, token, service) do
  with {:ok, endpoint_arn} <- MyApp.Push.Sns.register_token(token, service),
       nil                 <- Repo.get_by(PushToken, endpoint_arn: endpoint_arn)
  do
      user
      |> build_assoc(:push_tokens)
      |> PushToken.changeset(%{endpoint_arn: endpoint_arn})
      |> Repo.insert()
  else
    %PushToken{} = pushtoken -> {:ok, push_token}
    other                    -> other
end

I would recommend moving this function out into a module too.

2 Likes
4

Thank you, looks a bit nicer!

I should have added that I currently run Elixir 1.2.6 so I don’t have the else clause for the with construct.

1 Like
5

No problem:

def create_and_register_token(user, token, service) do
  with {:ok, endpoint_arn} <- MyApp.Push.Sns.register_token(token, service),
       nil                 <- Repo.get_by(PushToken, endpoint_arn: endpoint_arn)
  do
    user
    |> build_assoc(:push_tokens)
    |> PushToken.changeset(%{endpoint_arn: endpoint_arn})
    |> Repo.insert()
  end
  |> case do
    %PushToken{} = pushtoken -> {:ok, push_token}
    other                    -> other
  end
end
2 Likes
6

Nice!

So, what I did now was to split the functionality into two modules, a Push module and the Controller:

defmodule MyApp.PushTokenController do
  use MyApp.Web, :controller

  def create(conn, %{"push_token" => %{"service" => service, "token" => token}}) do
    case MyApp.Push.create_and_register_token(conn.assigns.current_user, token, service) do
      {:ok, _push_token} ->
        send_resp(conn, 201, "")
      {:error, %Ecto.Changeset{} = changeset} ->
        conn
        |> put_status(:unprocessable_entity)
        |> render(MyApp.ChangesetView, "error.json", changeset: changeset)
      {:error, message} ->
        conn
        |> put_status(:bad_request)
        |> render(MyApp.ErrorView, "400.json", message: message)
    end
  end
end


defmodule MyApp.Push do
  import Ecto, only: [build_assoc: 2]

  alias MyApp.PushToken
  alias MyApp.Repo

  def create_and_register_token(user, token, service) do
    with {:ok, endpoint_arn} <- MyApp.Push.Sns.register_token(token, service),
         nil                 <- Repo.get_by(PushToken, endpoint_arn: endpoint_arn)
    do
      user
      |> build_assoc(:push_tokens)
      |> PushToken.changeset(%{endpoint_arn: endpoint_arn})
      |> Repo.insert()
    end
    |> case do
         %PushToken{} = push_token -> {:ok, push_token}
         other                     -> other
       end
  end
end

The Controller looks quite nice to me! Much better than before, also the MyApp.Push module can easily be tested, though I do have some trouble reading the with code. But maybe that’s just because I’m not used to seeing it yet :slight_smile:

Do we have any other ideas on how to write this code?

1 Like
7 
def create(conn, %{"push_token" => %{"service" => service, "token" => token}}) do
  MyApp.Push.Sns.register_token(token, service)
  |>proc_arn(conn)
end

def proc_arn({:error, message}, conn) do
  conn
  |> put_status(:bad_request)
  |> render(MyApp.ErrorView, "400.json", message: message)
end
def proc_arn({:ok, e_arn}, conn) do
  if get_token(e_arn) || ({status,chset}=store_token(e_arn)) && status==:ok do
    send_resp(conn, 201, "")
  else
    conn
    |> put_status(:unprocessable_entity)
    |> render(MyApp.ChangesetView, "error.json", changeset: chset)
  end
end

def get_token(endpoint_arn), do: Repo.get_by(PushToken, endpoint_arn: endpoint_arn)

def store_token(endpoint_arn, conn)
  conn.assigns.current_user
  |> build_assoc(:push_tokens)
  |> PushToken.changeset(%{endpoint_arn: endpoint_arn})
  Repo.insert
end
1 Like
8

Look at the Happy library. It is Elixir’s 1.3 with, but better, and works on older Elixirs too.