Phoenix framework is a web framework for Elixir programming language, which became the go-to set of libraries when we start new project for our clients. It’s performant and robust, flexible and has sane defaults - we enjoy working with Phoenix a lot.

However nice and programmer-friendly the framework may be, there is always other pieces that build your application, and most important of these is your own code.

Code refactoring is a critical step during development of applications here at AmberBit. Whenever we have the pull request ready to be submitted and reviewed by other developers, we usually take a last look at the code we wrote and decide what to refactor and how. Everyone likes pretty code!

One of the most usual suspects that get picked as a part that needs refactoring are Phoenix controllers. Let’s have a look at several techniques we use to make our code look pretty.

Technique #1: Extract logic to Plugs

Plug is a library that Phoenix build on top of, and it provides basic interface to deal with HTTP requests and web servers in Elixir applications. Good place to start learning about Plug is this section of ElixirSchool.

Whenever you have code which repeats in multiple controller actions or simply has to be executed before each action - creating a custom plug that does the required job is the way to go.

Let’s have a look at the following controller:

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

  def index(conn, _params) do
    conn = conn
           |> assign(:current_user, find_current_user(conn))

    case conn.assigns.current_user do
      nil ->
        conn
        |> put_flash(:info, "Please sign in!")
        |> redirect(to: login_path(conn, :new))
        |> halt()

      _user ->
        conn
        |> render("index.html")
    end
  end

  ...

  defp find_current_user(conn) do
    ...
  end
end

We want to make sure we show the page only to signed in users. In all other cases we want to redirect to login page.

It’s fine to have such code in one place of application, maybe two - but if you have to call find_current_user/1 and conditionally branch off your program flow with case or another conditional syntax construct in every single controller and every single action (except login page) this quickly becomes tiresome.

The way I like to break it down is to introduce not one - but two custom plugs: one which finds currently signed in user, and another one that redirects user to login page if user is not signed in. This give us flexibility to have pages where being signed in is not required - but it can enable personalized / additional functionality - such as adding comments, and have other pages that require user to be signed in.

defmodule MyApp.Plugs.FindCurrentUser do
  import Plug.Conn

  def init(_opts) do
    :ok
  end

  def call(conn, _) do
    conn
    |> assign(:current_user, find_current_user(conn))
  end

  defp find_current_user(conn) do
    ...
  end

end

defmodule MyApp.Plugs.RequireCurrentUser do
  import Plug.Conn

  def init(_opts) do
    :ok
  end

  def call(conn, _) do
    case conn.assigns[:current_user] do
      nil ->
        conn |> redirect(to: login_path(conn, :new)) |> halt()

      _user ->
        conn
    end
  end
end

There are (at least) two ways to use our brand new plugs in controllers:

1. Directly instructing Phoenix to execute them - form within controllers themselves.
2. By adding these plugs to a pipeline in router

Let’s see how our controller looks like if we go with option 1:

defmodule MyApp.Web.PostsController do
  use MyApp.Web, :controller
  plug MyApp.Plugs.FindCurrentUser
  plug MyApp.Plugs.RequireCurrentUser

  def index(conn, _params) do
    conn
    |> render("index.html")
  end
end

Better? Better.

In case you want to exclude plugs executing from certain controller actions - or selectively specify before which actions plug should be executed, you can add guard clauses directly in your controllers too.

Usually more handy and less error-prone way than adding plugs to your stack in controllers is to alter your router to do so for you:

defmodule MyApp.Router do
  use MyApp.Web, :router

  pipeline :browser do
    plug :accepts, ["html"]
    plug :fetch_session
    plug :fetch_flash
    plug :protect_from_forgery
    plug :put_secure_browser_headers
    plug MyApp.Plugs.FindCurrentUser # add this line
  end

  pipeline :require_current_user do
    plug MyApp.Plugs.RequireCurrentUser
  end

  ...

  scope "/", MyApp do
    pipe_through [:browser]

    get "/login", LoginController, :new
    ...
  end

  scope "/", MyApp do
    pipe_through :browser
    pipe_through :require_current_user

    resources "/posts", PostsController
    ...
  end
  ...
end

As you can see above, you can alter your standard :browser pipeline to include plug that finds and assigns current user to conn.assigns.current_user, and create additional pipeline which ensures user is signed in. All the routes defined in the second scope block will redirect to login page if user is not signed in.

This way you can easily separate your public pages from these that require being signed in to account in your system.

The same technique can be applied to several other cases, such as:

• finding and assigning parent resources in case of nested resources such as /posts/:post_id/comments/:comment_id
• introducing authorization / permission checking based on controller/action/role given user has in the system
• setting language / locales based on cookie
• finding current tenant in multi-tenant system based on domain/subdomain

and many other similar use cases.

Technique #2: Extract logic to workflows (a.k.a. services)

Whenever your controllers become bloated, and you have multiple helper functions in them - good idea may be to move the business logic to separate modules. Doing so helps with testability and code re-use, but most importantly leaves the controllers to do one thing and one thing only: control how and what to render, depending on the outcome of some business logic process.

Personally I think best course of action, for most controllers and actions, is to delegate as much as you can, possibly each action - to it’s own module / function that defines it’s behavior.

Let’s consider the following controller:

defmodule MyApp.Web.RegistrationController do
  use MyApp.Web, :controller
  plug MyApp.Plugs.RequireNoUser # custom plug we learned above about
  plug :put_layout, "no_user.html"

  def new(conn, _params) do
    conn
    |> render("new.html", changeset: MyApp.User.changeset(%MyApp.User{}, %{}))
  end

  def create(conn, %{"registration" => registration_params}) do
    user_changeset = MyApp.User.changeset(%MyApp.User{}, registration_params)
    case MyApp.Repo.insert(user_changeset) do
      {:error, changeset} ->
        conn
        |> render("new.html", changeset: changeset))

      {:ok, user} ->
         user
         |> seed_example_dashboard_data()
         |> send_email_confirmation_request()
         |> send_admins_new_user_notirication()

         conn
         |> sign_in_user(user)
         |> redirect(to: dashboard_path(@conn, :show))
    end
  end

  defp send_email_confirmation_request(user) do
     ...

  defp send_admins_new_user_notification(user) do
     ...

  defp sign_in_user(user) do
     ...

  defp seed_example_dashboard_data(user) do
     ...
end

It’s a pretty standard signup controller, handling new user registration and onboarding. While it’s pretty standard - there is still plenty of things going on. Not only we validate the form (here using directly changeset and Ecto schema backed by database), but after user registered we have to seed their dashboard with example data, send them confirmation e-mail with a link, send notification to admins and sign user to the system. This could easily mean this single controller has already 200 lines of code or more. And that’s just a single action.

How I like to deal with bloated controllers is moving the business logic to reusable services. I have noticed these services often take a form of multi-step algorithms, and for a while I adopted naming convention of calling them “workflows”. I must admit, I stole the naming from another Elixir developer, but I think I also infected others with the idea since, which suggests it’s not terribly bad.

Let’s see how can we extract the business logic to a workflow:

defmodule MyApp.Registration.Workflow do
  def run(registration_params) do
    with user_changeset <- MyApp.User.changeset(%MyApp.User{}, registration_params),
         {:ok, user} <- MyApp.Repo.insert(user_changeset),
         :ok <- seed_example_dashboard_data(user),
         :ok <- send_email_confirmation_request(user),
         :ok <- send_admins_new_user_notirication(user) do
           generate_sign_in_token(user)
    end
  end

  defp send_email_confirmation_request(user) do
     ...

  defp send_admins_new_user_notification(user) do
     ...

  defp generate_sign_in_token(user) do
     ...

  defp seed_example_dashboard_data(user) do
     ...
end

And we can use it in controller like this:

defmodule MyApp.Web.RegistrationController do
  use MyApp.Web, :controller
  plug MyApp.Plugs.RequireNoUser # custom plug we learned above about
  plug :put_layout, "no_user.html"

  def new(conn, _params) do
    conn
    |> render("new.html", changeset: MyApp.User.changeset(%MyApp.User{}, %{}))
  end

  def create(conn, %{"registration" => registration_params}) do
    case MyApp.Registration.Workflow.run(registration_params) do
      {:error, changeset} ->
        conn
        |> render("new.html", changeset: changeset))

      {:ok, sign_in_token} ->
         conn
         |> sign_in_user_with_token(sign_in_token)
         |> redirect(to: dashboard_path(@conn, :show))
    end
  end

  defp sign_in_user_with_token(token) do
    ...
end

Better? I think so. Our controller is not concerned with generating token, sending e-mails nor dealing with database - it delegates all the hard work to dedicated workflow, while being focused on being a bridge between framework and your business logic. It’s almost like controllers are piece of infrastructure, and you can stop thinking about them as part of your “core” applications. Workflows/services are where the logic lives.

The bonus part about extracting business logic to workflows is that they are testable independently from controllers and Phoenix itself. You can test them in isolation.

Writing tests for workflows/services is usually easier and quicker to writing full controller tests or writing tests with something like Selenium/Wallaby combo.

What I like to do on the testing side of things is to write “happy path” tests using Wallaby, and leave edge cases to be tested with unit tests testing workflow itself. In the case of our example registration workflow and controller, I would write these Wallaby tests:

defmodule MyApp.Integration.RegistrationTest do
  ...

  test "should register and sign in user if they filled in form correctly"

  test "should display validation errors and not register user if the form is incorrect"
end

and then add fasts tests that do not rely on real browser to test some edge cases:

defmodule MyApp.Registration.WorkflowTest do
  ...

  test "should insert new user to database after successfull registration"

  test "should generate and send e-mails to user and admins"

  test "should generate sign in token to be stored in the cookie"

  test "should require e-mail, first and last name"

  test "should require e-mail in valid format"

  test "should make sure e-mail is unique in the system"
end

Technique #3: Using tableless Ecto schemas for custom form validations

In the examples above we have extracted some business logic to custom plugs, and then extracted more business logic to workflows. Our registration workflow is now dealing with database directly to perform Repo.insert(changeset), but we still have to initialize the form with:

...

  def new(conn, _params) do
    conn
    |> render("new.html", changeset: MyApp.User.changeset(%MyApp.User{}, %{}))
  end

...

This is not ideal for two reasons:

1. Our controllers need to know about our Ecto schemas. You can argue that’s not a big issue, and I’ll agree, but it’s always nice to have separation of layers in your application where upper layer does not necessarily reach the bottom layer, but goes through the intermediary. In our case we have introduced workflow/services layer that we use create action, but not in new.

2. We are cluttering our Ecto schemas with business logic. This is a bigger problem, because it can possibly lead to security issues, where we unwillingly expose some database fields to be public where in fact we want them to be protected from user writes.

Let’s focus on reason 2 for a while. If our User model has is_admin field, which determines their system-wide administrative rights, and our User.changeset/2 function allows setting this field, we have a possible security vulnerability in our registration form. Even though the form we prepared has no field for is_admin in it, it doesn’t mean a hacker can’t insert a field in the form. This can be as simple as right-clicking in Chrome on the page, going to Developer Tools and writing a single line of code to our HTML form:

<input type="hidden" name="user[is_admin]" value="true" />

User can register and obtain illegitimately admin rights. Oops. Yup.

There are several ways to deal with that. You could create a changeset that does not accept is_admin field. But then, you may need to have a place where user can grant these permissions to others. If this need arises, you end up with two changeset functions, that you use from different parts of the code.

While the solution with multiple changesets is okay, I like to introduce custom, tableless schemas to take care of parameters filtering and validation.

We can extract the type casting and form validation from our MyApp.User module to something like MyApp.Registration.Form and use it in our workflow and controller.

defmodule MyApp.Registration.Form do
  use Ecto.Schema
  import Ecto.Changeset

  embedded_schema do
    field(:first_name, :string)
    field(:last_name, :string)
    field(:email_address, :string)
    field(:short_bio, :string)
  end

  @required_fields [:email_address, :first_name, :last_name]
  @optional_fields [:short_bio]

  def changeset(attrs) do
    %__MODULE__{}
    |> cast(attrs, @required_fields ++ @optional_fields)
    |> validate_required(@required_fields)
    |> validate_format(:email_address, ~r/(\w+)@([\w.]+)/)
    |> validate_not_already_registered()
  end

  defp validate_not_already_registered(changeset) do
    with nil <- changeset.errors[:email_address],
         %MyApp.User{} = user <- find_existing_user_by_email(changeset.changes.email_address)
      changeset
      |> add_error(:email_address, "is already taken")
    else
      _ ->
        changeset
    end
  end

  defp find_existing_user_by_email(email) do
    ...
end

defmodule MyApp.Web.RegistrationController do
  ...

  def new(conn, _params) do
    conn
    |> render("new.html", changeset: MyApp.Registration.Form.changeset(%{}))
  end

  ...
end

In workflow itself we have to do one more thing in order the Phoenix helpers to show validation errors: set the action field on the changeset manually, but other than that we can just take changeset.changes from it and feed directly to User.changeset:

 defmodule MyApp.Registration.Workflow do
  def run(registration_params) do
    with %{valid?: true} = form_changeset <- MyApp.Registration.changeset(registration_params),
         db_changeset <- MyApp.User.changeset(%MyApp.User{}, form_changeset.changes),
         user <- MyApp.Repo.insert!(db_changeset),
         :ok <- seed_example_dashboard_data(user),
         :ok <- send_email_confirmation_request(user),
         :ok <- send_admins_new_user_notirication(user) do
           generate_sign_in_token(user)
    else
      %{valid?: false} = changeset ->
        {:error, %{changeset | action: :create}}

      error ->
        error
    end
  end

  ...
end

And, quite likely we want to alter the form template to specify “as” option for the form helper:

<%= form_for @changeset, Routes.registration_path(@conn, :create), [as: :user, method: :post], fn f -> %>
  ...

Final thoughts

We like to keep our controllers and Ecto schemas lean and as much business-logic-free as possible. Both of these parts of application are where your own code deals with infrastructure. It makes sense from code reliability, security and ease of future upgrades to make them as simple as possible - and move your business logic to some other place.

Refactor bravely and refactor often, my Elixir friends :).

Post by Hubert Łępicki

Hubert is partner at AmberBit. Rails, Elixir and functional programming are his areas of expertise.