That somebody said something is anti-pattern is not enough to convince me. I’ve seen the alternative way of doing things and spraying many small functions inside your module does not look like an objective improvement to me. Tracking code visually becomes harder and you still have to massage returned error values. For what? To not have else? Why is that a selling point? At least you have explicitly spelled out all error states which is a clear win in dynamic languages.
dimitarvp
0
D4no0
No, to not have to write defensive code. In a lot of cases you don’t care about the error shape, nor you care about handling it, you just care about the happy path.
As it was said countless times before, these anti-patterns are just generic guidelines, the only thing that I value in a codebase is consistency. If you went with an approach, make sure you use it consistently, otherwise this becomes amateur hour.
1
dimitarvp
We’re working on different projects for sure, because in my work letting an error escape means a background job with 15+ dependencies does not get to run [on time] and then you have to go manually and fix things… or buy Oban Pro, which my employer still hasn’t done (because the value proposition for them is not clear).
For a lot of projects you can just indeed leave the happy path only and go fix stuff only when you see a blow-up in your APM system. For those I work on – nope.
Yeah, I 100% agree with this. Pick a style, stick to it, explain why (document somewhere for bonus points), profit.
1
Lucassifoni
If we are at an user-facing edge of the system, the different steps could generate different error messages or even instructions for other flows of the system.. but that also could be handled without this multi-clause else.
Two days ago I tried to see my “self-teaching wallet” status, a French mandatory tax that gets deposited into a state wallet for you to buy overpriced courses by shady organisations afterwards.
I had to change my password as I did not log in for a while.
The password had a restriction of having 12 chars, including symbols. The symbol “greenlight” was represented by an ampersand &. I used Firefox’s “generate a secure password” feature and did not even look at it.
Password gets changed, I get a confirmation e-mail, I try to log in.
“Something wrong happened with a system”. Duh. I open the inspector tab to see that instead of a json response describing the error, I got an HTML response from a WAF with “SQL Injection attempt detected”.
I display the password, and it was generated with “&&” inside it. I remove an “&”, click again, and get an “Incorrect password notice”. Change the password again, without the “&&” that was deemed acceptable by the password change wizard, and I log in 
.
My point being, when we’re in a controller meant to be interacted with by a real person, handling all cases with explicit error messages along with error-specific codes to pass around to support is a real benefit.
Multi-clause else can be a pattern to concisely dispatch those situation-specific, user-consumed error payloads. But that depends on the codebase.
1
D4no0
There are multiple ways to achieve the same solution. The one I personally like the most is using fallback controller. That one is not only the most readable, but it also separates system errors from user-facing errors.
I think its worth pointing out that there is no universal solution that has no drawbacks ultimately, and if the solution you came with (like with with multiple else clauses) works very well, there is no reason not to use it.
In terms of code styling, I think elixir is very lean (I would say too lean sometimes) and you can see a huge variety of coding styles from people with different backgrounds. I am obviously biased towards one of them.
2
mpope
While Elixir folks seem to sometimes balk at the lowly use of chaining together function heads and pattern matching, I like to use them over with in cases like this. Its less compact, but you get flexibility without having to think of tagging or overlapping errors and whatnot:
def onboard(conn, params) do
handle_verification(verify_zendesk_origin(conn))
end
def handle_verification({:error, _reason}) do
conn
|> put_status(:forbidden)
|> json(%{error: "Invalid origin"})
|> halt()
end
def handle_verification({:ok, origin}) do
user_params = params["user"]
organisation_attrs = %{
subdomain: params["subdomain"],
name: params["name"],
public_key: params["public_key"]
}
handle_org_upsert(Organisations.upsert(organisation_attrs), user_params)
end
def handle_org_upsert({:error, changeset}, _) do
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
def handle_org_upsert({:ok, organisation}, user_parms) do
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
}
handle_user_upsert(ExternalAccounts.upsert_user(user_attrs))
end
def handle_user_upsert({:error, changeset}) do
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
def handle_user_upsert({:ok, user}) do
conn
|> put_status(:ok)
|> json(%{user_id: user.id})
|> halt()
end
0
derek-zhou
Actually, I think your current code is just fine. Real world business logic is messy by nature and constantly evolving; abstractions will make later modifications unnecessarily hard. I will just confine the mess in a few known “messy” modules and call it a day.
1
Sorc96
I really don’t like this, because in order to get a high level understanding of what steps happen as part of onboarding, I have to read everything including all the details.
0
wpiekutowski
Here’s another take how to improve this without changing the interface of your functions. It uses a result type {:ok, any()} | {:error, :invalid_origin | %Ecto.Changeset.t()} to handle errors:
and_then(result, fun)when result is{:ok, ok_value}, calls the function with anok_value. Otherwise it passes through the error result. Function is supposed to return a result type.map_error(result, fun)when result is{:error, error_value}, calls the function with an error value or passes through the ok result. Function is supposed to return a value.
def onboard(conn, params) do
verify_zendesk_origin(conn)
|> map_error(fn _reason -> :invalid_origin end)
|> and_then(fn _origin -> params_to_org_upsert(params) end)
|> and_then(&Organisations.upsert/1)
|> map_error(fn changeset ->
ChangesetError.new!(
changeset,
label: "organisation upsert (onboard) changeset errors",
message: "invalid organisation data"
)
end)
|> and_then(¶ms_and_org_to_upsert_user(params, &1))
|> and_then(&ExternalAccounts.upsert_user/1)
|> map_error(fn changeset ->
ChangesetError.new!(
changeset,
label: "user upsert (onboard) changeset errors",
message: "invalid user data"
)
end)
|> case do
{:ok, %User{} = user} ->
conn
|> put_status(:ok)
|> json(%{user_id: user.id})
|> halt()
{:error, :invalid_origin} ->
conn
|> put_status(:forbidden)
|> json(%{error: "Invalid origin"})
|> halt()
{:error, %ChangesetError{} = e} ->
IO.inspect(e.changeset.errors, label: e.label)
conn
|> put_status(:unprocessable_entity)
|> json(%{
error: e.message,
details: changeset_error_to_string(e.changeset)
})
|> halt()
end
end
def and_then({:ok, value}, fun), do: fun.(value)
def and_then({:error, _} = error, _fun), do: error
def map_error({:ok, _} = ok, _fun), do: ok
def map_error({:error, error_value}, fun), do: {:error, fun.(error_value)}
defmodule ChangesetError do
@keys [:changeset, :label, :message]
@enforce_keys @keys
defstruct @keys
def new!(%Ecto.Changeset{} = changeset, label: label, message: message)
when is_binary(label) and is_binary(message) do
%__MODULE__{changeset: changeset, label: label, message: message}
end
end
defp params_to_org_upsert(params) do
{:ok, %{
subdomain: params["subdomain"],
name: params["name"],
public_key: params["public_key"]
}}
end
defp params_and_org_to_upsert_user(params, organisation) do
user_params = params["user"]
{:ok, %{
external_id: to_string(user_params["id"]),
name: user_params["name"],
role: user_params["role"],
avatar_url: user_params["avatarUrl"],
ogranisation_id: organisation.id
}}
end
Consider modifying verify_zendesk_origin to only have what it actually needs from conn, instead of the whole thing.
Depending on personal taste, you might prefer to not have params_*_to_* and do the mapping inside an anonymous function.
verify_zendesk_origin(conn)
|> map_error(fn _reason -> :invalid_origin end)
|> and_then(fn _origin ->
%{
subdomain: params["subdomain"],
name: params["name"],
public_key: params["public_key"]
}
end)
|> and_then(&Organisations.upsert/1)
You might also consider moving everything about orgranisation upsert and user upsert into separate functions:
def onboard(conn, params) do
verify_origin(conn)
|> and_then(&upsert_organisation(&1, params))
|> and_then(&upsert_user(&1, params))
|> case do
{:ok, %User{} = user} ->
conn
|> put_status(:ok)
|> json(%{user_id: user.id})
|> halt()
{:error, :invalid_origin} ->
conn
|> put_status(:forbidden)
|> json(%{error: "Invalid origin"})
|> halt()
{:error, %ChangesetError{} = e} ->
IO.inspect(e.changeset.errors, label: e.label)
conn
|> put_status(:unprocessable_entity)
|> json(%{
error: e.message,
details: changeset_error_to_string(e.changeset)
})
|> halt()
end
end
def verify_origin(conn) do
verify_zendesk_origin(conn)
|> map_error(fn _reason -> :invalid_origin end)
end
def upsert_organisation(_, params) do
%{
subdomain: params["subdomain"],
name: params["name"],
public_key: params["public_key"]
}
|> Organisations.upsert()
|> map_error(fn changeset ->
ChangesetError.new!(
changeset,
label: "organisation upsert (onboard) changeset errors",
message: "invalid organisation data"
)
end)
end
def upsert_user(organisation, params) do
user_params = params["user"]
%{
external_id: to_string(user_params["id"]),
name: user_params["name"],
role: user_params["role"],
avatar_url: user_params["avatarUrl"],
ogranisation_id: organisation.id
}
|> ExternalAccounts.upsert_user()
|> map_error(fn changeset ->
ChangesetError.new!(
changeset,
label: "user upsert (onboard) changeset errors",
message: "invalid user data"
)
end)
end
This way the next developer coming to see what onboard actually does, clearly sees what are the major steps. Of course it makes sense to put
verify_origin(conn)
|> and_then(&upsert_organisation(&1, params))
|> and_then(&upsert_user(&1, params))
into an application service module. It’s best not to pass all params or whole conn, but ideally a struct with only the required data.
Debugging this kind of pipelines is easy with Elixir’s dbg function. You can clearly see what each step returned.
1
camstuart
Thanks for explaining that @wpiekutowski ! I had hoped that there was a pipeline style approach, but I had not been able to see how to handle errors. But your examples clearly show that.
Sometimes Elixir reminds me of my Perl days. There is more than one way to do it!
I will give your design a try, it seems the most “Elixir-ish” so far.
Good point about not using all of conn and params thats just a bit of laziness on my part
0