I reckoned it is not because the else clauses use the conn variable. I could pass those to the private functions parse_id/1 and fetch_book/1, but not sure if that is recommended…
# Part of an elixirland.dev exercise
defmodule BookClubWeb.APIController do
use BookClubWeb, :controller
alias BookClub.Books
def books(conn, %{"title" => title}) do
books = Books.list_books_with_active_or_first_page(filter: title)
render_pretty_json(conn, books)
end
def books(conn, _params) do
books = Books.list_books_with_active_or_first_page()
render_pretty_json(conn, books)
end
def book(conn, %{"id" => id}) do
with {:ok, id} <- parse_id(id),
{:ok, book} <- fetch_book(id) do
render_pretty_json(conn, book)
else
{:error, :invalid_id} ->
conn
|> put_status(:bad_request)
|> json(%{error: "Invalid ID"})
{:error, :not_found} ->
conn
|> put_status(:not_found)
|> json(%{error: "Book not found"})
end
end
defp render_pretty_json(conn, data) do
json_string = Jason.encode!(data, pretty: true)
conn
|> put_resp_content_type("application/json")
|> send_resp(200, json_string)
end
defp parse_id(id) do
case Integer.parse(id) do
{id, _} when id >= 1 -> {:ok, id}
_ -> {:error, :invalid_id}
end
end
defp fetch_book(id) do
case Books.get_book_with_active_or_first_page(id) do
nil -> {:error, :not_found}
book -> {:ok, book}
end
end
end
It seems borderline to me. Even though the errors are pretty easy to map back to the different with clauses, it is still extra “strain” rather than handling the different types of errors in the caller.
defmodule BookClubWeb.APIController do
use BookClubWeb, :controller
alias BookClub.Books
def books(conn, %{"title" => title}) do
books = Books.list_books_with_active_or_first_page(filter: title)
render_pretty_json(conn, books)
end
def books(conn, _params) do
books = Books.list_books_with_active_or_first_page()
render_pretty_json(conn, books)
end
def book(conn, %{"id" => id}) do
with {:ok, id} <- parse_id(id, conn),
{:ok, book} <- fetch_book(id, conn) do
render_pretty_json(conn, book)
end
end
defp render_pretty_json(conn, data) do
json_string = Jason.encode!(data, pretty: true)
conn
|> put_resp_content_type("application/json")
|> send_resp(200, json_string)
end
defp parse_id(id, conn) do
case Integer.parse(id) do
{id, _} when id >= 1 ->
{:ok, id}
_ ->
conn
|> put_status(:bad_request)
|> json(%{error: "Invalid ID"})
end
end
defp fetch_book(id, conn) do
case Books.get_book_with_active_or_first_page(id) do
nil ->
conn
|> put_status(:not_found)
|> json(%{error: "Book not found"})
book ->
{:ok, book}
end
end
end
Sorry on phone so can’t type out code, but I think in your case I would take the conn stuff out of the parse and fetch functions. I would have the book function return either ok or error tuple and then do the conn stuff on the return from that function. That way you are talking away the need to ever care about which error comes from which with clause. You just know the book function can return different kinds of errors
I think this solution is harder to follow. I do not like to see a function called “parse_id” modifying conn. Explicit error case clauses in the first code sample seems much clearer.
In this case, I’d have the helper functions return a 3-tuple in the error case, e.g. {:error, status_code, message} and a single else block to update the conn
Maybe it’s copy pasta, but that last function clause def book(conn, %{"id" => id}) do won’t match as there’s a “catch all” def books(conn, _params) do right before it. Order matters here since Elixir tries each clause until it finds one that matches so that “catch all” should come last.
I would stick to your first version. It is clear what the returned errors are, the code is clean and concise.
Introducing functions to have functions is overkill.
The idea behind the anti-pattern is when you have multiple functions returning the same error tuples for different situations. If you have 2 fetch functions that both return {:error, :not_found} then you don’t know where it came from, that has to be avoided.
Therefore the suggestion is to return distinct tuples, which you already do.
It will always be a matter of taste. I for example have a with clause with 9 different else-clauses but they all differ and are clear where they originate.
I think this is right, and this kind of a thing is a challenge with a lot of anti-patterns. Lots of clauses adds complexity, but so do functions, so it’s not the case that simply replacing one with the other will always result in simpler code that all experienced developers agree is better/more maintainable. I know Jose has been trying to pare down that list to just the most important and objective pain points, but I almost feel like the top of that doc needs a big YMMV so people new to Elixir don’t take it too seriously and worse, try to use it as a club against other devs.
I would go so far as to say that you should never return 3-tuples when returning ok/error results. In this case it’s high-level in a controller, so maybe this is best-case for it, but oftentimes when the error is returned you might end up with an error handler five levels up where the 3-tuple was passed up and the higher level code is (reasonably) expecting just {:ok, _} or {:error, _}. Much better habit to always stick to those two patterns and if you need two bits then putting a tuple in your error like {:error, {_, _}}.
But another possibility, if you’re getting that complex, is to return an exception struct as the value in your error tuple (a pattern used by libraries like httpoison, mint, postgrex, and others). If you do that relatively often then you can look for them and call Exception.message to get a string while keeping the error logic inside of the exception struct module.
I also support the original code, by the way. Much better to have parse_id / fetch_book be focused on one thing rather than two things (vis-a-vis separation-of-concerns).
What I usually do is use fallback controller using action_fallback/1 for this:
defmodule MyFallbackController do
use Phoenix.Controller
def call(conn, {:error, :not_found}) do
conn
|> put_status(:not_found)
|> put_view(MyErrorView)
|> render(:"404")
end
def call(conn, {:error, :unauthorized}) do
conn
|> put_status(:forbidden)
|> put_view(MyErrorView)
|> render(:"403")
end
end
In this way you have a single place where you handle all the potential errors and you can use with in your codebase without any concerns on intermediate error handling. As long as your return value != conn from your plugs, the fallback action will be always triggered.
I’m not willing to go that dogmatic, but otherwise, yeah, i generally agree with you. I’ve felt that pain and I even gave an ElixirConf talk a couple of years ago about exceptions and advocated the {:error, exception} approach.
I’d be equally happy with {:error, {status_code, message}} in this isolated situation.
def book(conn, %{"id" => id}) do
with {:ok, id} <- parse_id(id),
{:ok, book} <- fetch_book(id) do
render_pretty_json(conn, book)
else
{:error, error} ->
handle_error(conn, error)
end
end
defp handle_error(conn, :bad_request) do
conn
|> put_status(:bad_request)
|> json(%{error: "Invalid ID"})
end
defp handle_error(conn, :not_found) do
conn
|> put_status(:not_found)
|> json(%{error: "Book not found"})
end
My reasoning:
you have two simple functions, a happy path function and a sad path function, instead of one function handling both.
I wouldn’t want the functions returning errors to tell me what their status is. Their job is to do the thing or tell me something went wrong, not to provide an HTTP status. Thats the controller action’s job.
it lets the programmer looking at the code not think about errors if they aren’t what they are here for, but they can see where errors are handled.
I am happy with that whenever (call me dogmatic if you like ). I have found that strictly adhering to two-tuples for results (ok / error) and then deconstructing the 2nd element if/when the need calls for it leads to much more predictable code.
The complexity involved in manually emulating exhaustive pattern matching – something that Erlang / Elixir sadly don’t have at compile-time – spirals out of control (and of the grasp of the human brain) extremely quickly. Some dogma is actually pragmatic and very helpful there.
I hope my initial statement didn’t come off harsh. There’s a little rebellious voice in my head that reacts when people say “always do …” or “never do …” and it looks for situations when that advice is unwise.
If I saw the original code in a PR I was reviewing, I wouldn’t even comment on it. It’s good enough and isolated within a single module. If you’re really feeling the pain of diverse patterns of result tuples (and I have worked in such systems), then I advise custom exception structs. Plus you can use those module to house additional error management functions if desired. I won’t advise to always use {:error, exception} pattern because it’s overhead and there’s tradeoffs to consider.
Understanding that there are tradeoffs in any specific circumstance is what I consider pragmatic. I’d even argue that once an anti-pattern is identified, it doesn’t automatically mean not to do it. Sometimes it can be justified in a given situation.
It absolutely did not, I just felt the sudden urge to joke with you.
Exactly the same here, and as I get older I wish I was less rebellious or, to put it in an even better perspective – wish I knew when to be and when not to be.
Well I have PTSD from C/C++, Java, Ruby, PHP and who knows what else so I started subconsciously avoiding exceptions in Elixir from the get go. This has not stopped me writing rock-solid apps that can literally only be brought down by a SIGKILL or the OOM reaper but using exceptions in my own code is another matter entirely – and I don’t use them.
But it’s likely that I am wrong to be so against them. No idea.
True, agreed. At the same time both you and me have decades of programming experience under our belts so there’s also the extra context that some practices almost always end up costing more than they benefit you so you learn to avoid them most of time. That’s what I do.
Agreed x2, that’s why I was mostly against the now-official Elixir list of anti-patterns. A lot of novices just lack the experience and critical thinking to tell themselves the following:
…and they become dogmatic themselves. Sad story but what can you do. Also off-topic.
). I have found that strictly adhering to two-tuples for results (ok / error) and then deconstructing the 2nd element if/when the need calls for it leads to much more predictable code.
