Pyramid of doom

Hi All,

I’m working on a backend system with an Elixir API that will be consumed by a separate phoenix app. Just about every function needs to return an :ok/:error tuple. I am ending up writing hideous functions, such as this one:

def get(id, %{user_id: user_id}) do
    # fetch user
    case Repo.get(SBCore.User, user_id) do
      nil -> {:error, "user not found"}
      user ->

        # fetch request list
        case Repo.get(RequestList, id) do
          nil -> {:error, "request list not found"}
          request_list ->

            # check permissions
            case can_view?(user, request_list) do
              false -> {:error, "insufficient permissions"}
              true ->

                # return data
                {:ok, request_list}
            end
        end
    end
  end

I’m relatively new to Elixir, but I know enough to know that this isn’t pretty code. How do I save myself from the pyramid of doom? I thought maybe with was the answer, but I haven’t been able to make that work.

Another idea I had was to write smaller functions that conform to a standard pipeable interface, similar to how Ecto changesets work. Essentially I would pass a struct along which includes fields like valid? and errors along with data. If any validation along the way errors out, the remaining validations in the pipeline would just short circuit. Then I would only need one case and the end of the pipeline to handle the output. I think this would be workable, but it may be over-engineered.

Any thoughts or suggestions would be appreciated.

Thank you!

1 Like

Seems like you want with/else from Elixir 1.3 along with perhaps a better return than Repo.get/2 gives you:

def please_get(mod, id, error_msg) do
   case Repo.get(mod, id) do
      nil -> {:error, error_msg}
      thing -> {:ok, thing}
   end
end
def get(id, %{user_id: user_id}) do
   with {:ok, user} <- please_get(SbCore.User, user_id, "User not found"),
          {:ok, request_list} <- please_get(RequestList, id, "Request_list not found"),
          true = can_view?(user, request_list) do
         {:ok, request_list}
   else
        {:error, msg} -> {:error, msg}
   end
end

There’s probably a better way to do this that involves structuring the overall code differently. Your get/2 function is doing a lot of work that I don’t think it ought to be doing, I’m guessing to save the client code from having to do some work that seems repetitive. That’s usually a bad idea—it really messes with the composability of your functions. A better approach would be to do the lookups of your data outside get/2 and pass in the dependencies. then you could use pattern matching to handle things more succinctly:

def get(nil, _), do: {:error, "User not found"}
def get(%SBCore.User{}=user, nil), do: {:error, "Request list not found"}
def get(%SBCore.User{}=user, %RequestList{}=list) do
   if can_view?(user, list) do
      {:ok, list}
   else
      {:error, "insufficient permissions"}
   end
end

#elsewhere...
with user <- Repo.get(SBCore.User, user_id),
     list <- Repo.get(RequestList, id) do
    get(user, list)
end
2 Likes

Thanks @karmajunkie!

You are right - I think with will work well here, potentially even without else. I had considered creating functions that allow better return values for each piece, but I hadn’t considered including a custom error message in the function signature. That is the key that makes this work.

As for structuring the code, I’m not sure the approach you mentioned will work in this case. The function get/2 is the API boundary of my OTP Application. It retrieves an object from the database by it’s ID after checking permissions. It is called by a separate Phoenix Application, which knows nothing about how data is persisted (no Repo access). As such, Phoenix can’t pass in the data because it has no way to get it other than to call this function.

Rather than push functionality up the call stack, I think I need to push downward. get should be responsible for orchestrating the flow only, and all data retrieval and evaluation of permissions, etc. should be handled by separate functions. Does that make sense?

1 Like

I think I see your objection to the second proposal, but consider that get/2 in this case can still be a fixed interface for client code, while calling functions within the OTP application which implement the outlined proposal. You’ll just be giving it a different name under the covers, which makes it a significantly cleaner version of the first proposal.

1 Like

Here is what I ended up going with:

  def get(id, %{user_id: user_id}) do
    with {:ok, user} <- Repo.fetch(SBCore.User, user_id, "user not found"),
         {:ok, request_list} <- Repo.fetch(RequestList, id, "request list not found"),
         :ok <- check_permissions(:view, request_list, user),
     do: {:ok, request_list}
  end

I implemented your please_get function as Repo.fetch

Thanks again for the guidance!

3 Likes