Help with idiomatic Elixir style when dealing with logic flow

camstuart

Hello,

I am somewhat new to Elixir, and finding that I am having difficulty grasping how I should handle logic for a series of sequences in an “operation”. For example, I have a phoenix post controller that I am using to onboard an organisation and user. So there are a few steps.

Verify where the request came from (I know this is not perfect)
Create an organisation if one does not already exist
Create a user for this organisation if one does not already exist

In this example I am relying on “halt” to essentially return early. But I think I might be approaching the problem in Elixir like an imperative language. But in a regular function where such a mechanism does (no return statement in the language) I get a bit lost on how I should manage control flow. This seems it should be broken up somehow.

I also have ended up with a rather “nested” outcome, which perhaps could (or should?) be avoided with that cool pipeline operator, but I don’t know how I would handle the unhappy path nicely.

I would really appreciate some feedback, and any learning resources that help people like me who have been working in imperative languages so long that we have trouble breaking the habit! Thanks for reading!

def onboard(conn, params) do
    case verify_zendesk_origin(conn) do
      {:ok, _origin} ->
        user_params = params["user"]

        organisation_attrs = %{
          subdomain: params["subdomain"],
          name: params["name"],
          public_key: params["public_key"]
        }

        case Organisations.upsert(organisation_attrs) do
          {:ok, organisation} ->
            user_attrs = %{
              external_id: to_string(user_params["id"]),
              name: user_params["name"],
              role: user_params["role"],
              avatar_url: user_params["avatarUrl"],
              organisation_id: organisation.id
            }

            case ExternalAccounts.upsert_user(user_attrs) do
              {:ok, user} ->
                conn
                |> put_status(:ok)
                |> json(%{user_id: user.id})
                |> halt()

              {:error, changeset} ->
                IO.inspect(changeset.errors, label: "user upsert (onboard) changeset errors")

                conn
                |> put_status(:unprocessable_entity)
                |> json(%{
                  error: "invalid user data",
                  details: changeset_error_to_string(changeset)
                })
                |> halt()
            end

          {:error, changeset} ->
            IO.inspect(changeset.errors, label: "organisation upsert (onboard) changeset errors")

            conn
            |> put_status(:unprocessable_entity)
            |> json(%{
              error: "invalid organisation data",
              details: changeset_error_to_string(changeset)
            })
            |> halt()
        end

      {:error, _reason} ->
        conn
        |> put_status(:forbidden)
        |> json(%{error: "Invalid origin"})
        |> halt()
    end
  end

31 comments

/phoenix #code-style

0 1278 31

2024-09-27 11:15:11 UTC

Showing Posts 1 to 10

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

Hermanverschooten

I would definitely go for a with in this case. Take a look at this anti-pattern, and use it as an example. Create functions that return either an :ok-tuple or a specific :error-tuple (or triplet) for that case.

Post #1

Lucassifoni

The logic could be extracted to an use-case module.
The origin verification could also be a plug.

At a very high and very verbose level :


plug YourAppWeb.Plugs, :verify_zendesk_origin when action in [:onboard, ...]
alias YourAppWeb.UserFlows.OnboardUser

def onboard(conn, params) do
    with {_, {:ok, organisation}} <- {:upsert_organisation, OnboardUser.upsert_organisation(conn.assigns.organisation_attrs)},
            {_, {:ok, user_attrs}} <- {:user_attrs, OnboardUser.user_attrs_from_params_and_org(params, organisation)}
            {_, {:ok, user}} <- {:upsert_external_user, OnboardUser.upsert_external_user(user_attrs)} do
         conn |> put_status(:ok) |> json(%{user_id: user.id})
   else
     {:upsert_organisation, {:error, e}} -> # handle this case
     {:user_attrs, {:error, e}} -> #handle this one
     {:upsert_external_user, {:error, e}} # handle this one
     _ -> # etc
    end
end

I’ve put this fictional OnboardUser module in YourAppWeb because the helper function user_attrs_from_params_and_org depends on the params arg, so it is linked to the transport.
You can of course refine that and have a pure non-web logic OnboardUser use-case while having other extracted utilities to properly construct the arguments it consumes from the request.

There also are a few different ways to tag error tuples or triples with with.
I like to do it at the call site to keep the logic free of this tagging.

The more non-web your logic is, the more testable it becomes

Edit : use-case based modules are an opinionated choice and not the idiomatic choice.

Post #2

Hermanverschooten

I do not like the tagged approach.
I would go for a function that returns a {:ok, organisation} or {:organisation_error, error_information}.
But I do like moving the initial verification to a plug.

Post #3

Lucassifoni

I think your way is cleaner overall, I don’t like putting the tags in the logic module, but have to admit it makes sense if we go for thin controllers.

Post #4

dimitarvp

Why not? It gets the job done.

Post #5

Lucassifoni

Tags at the call site make the real logic less noisy and aware of callers, and tags in the logic makes the caller leaner but aware of the tags… with use-case based organisation, both solutions shouldn’t really be problems.

In the end it depends of the style of the particular codebase you’re working on, the best solution might be to go with the flow of the rest of the code.

Post #6

stefanluptak

I would probably do something like this.

Usually, there are few types of errors that your web layer will handle.

Something like:

{:error, :not_found}
{:error, changeset}
{:error, "Some error message as string"}

If your context functions always return these, you can have your error handling in the fallback controller and then just do those nice with {:ok, something} <- YourContext.some_fun(params) calls. And if there’s some exception to that, you can handle it in the else clause of the with statement.

Post #7

camstuart

Wow, really great options by everybody, thanks so much!

I wondered about making a plug for verify_zendesk_origin there are a total of two actions in this controller that care about it, so I will definitely do that. Definately seems more testable and “out of the way” of the controller action itself.

These tags are rather interesting, I find this function you have written to be very clear and compact, I had not seen tags before. I’m reading up on with which seems to be the common suggestion. That confuses me a little, mainly because I use that in Python all the time, but it’s very different in Elixir!

Post #8