Refactoring - Part 1

Just like last year, Q4 is usually spent with improving Stencil quality including fixing bugs, refactoring, and removing technical debts.

I did a big refactor on the core of Stencil - image generation. It took me couple of weeks to come with a good refactor that I am happy with. The results are,

• standardized image generation process

Everything is done one place now. With this new design, I could hook easily to image generation process like adding post-image generation process i.e. calling webhooks, etc in one place rather than writing an adhoc code.

• mockable design with tests

I finally had tests written for the core functionality of Stencil :D

• reduce code duplicates

There were a lot of places where this logic were duplicated. Then, there's also code duplication between running Stencil in development and production.

In this article, I want to share how we use builder pattern in order to clean up the code base.

In the following article, I'll cover more on

• Single responsibility principles
• Function composition
• Pattern matching (and its anti-pattern)
• Railway/Happy path
• Error handling

Builder pattern

Builder is a pattern that allows you to construct a complex object step by step. The whole idea of builder pattern is having an object that you can set its properties to build various types of objects.  Each time you set some property on this object, it returns the modified object.

You can also use this pattern to construct an object that holds related data as part of its fields or properties. This object can be passed through a pipeline of functions that set various fields. This is how Ecto.Changeset work.

Although the builder pattern came from OOP, it is an excellent pattern in functional programming because of function composition and immutable data structure. In Elixir, this looks really elegance with pipeline operator (|>).

def registration_changeset(user, attrs, opts \\ []) do
	user
	|> cast(attrs, [:email, :password, :locale])
	|> validate_email()
	|> validate_password(opts)
end

Refactoring with

Let's take a look at how we clean up Stencil's code base to use this pattern.

Before we generate an image, we have an image pipeline where a request is validated and processed. This process includes attaching some metadata for internal tracking and building a template based on user's request.

Initially, using with is great for this purpose because the request can go through a linear process and when an error is returned, it short-circuited the rest of the process. However, there are few things that make me feel uneasy just by reading the code and which I think it could be improved.

Issues with with

1. Locating an error is harder

This can be solved with wrapping the function with a tuple, with the first element acts as an identifier.

{:validate_request, {:ok, request}} <- {:validate_request, Stencil.Templates.ImageRequestSync.validate_request(params)}

Then, we can pattern match on the first element to identify where the error is. This is very useful when some functions return various error, i.e. {:error, _} vs nil.

Of course, you could argue that we can wrap this function with a helper function that standardize the error.

2. Harder to read

This is actually my biggest gripe with this, especially combined with the solution to no. 1 above. It just makes the code harder to read, thus not so obvious to reason with.

Take a look at the code below, it's not easy to tell what's going on. The code is littered with atoms everywhere.

with {:validate_request, {:ok, request}} <-
	   {:validate_request, Stencil.Templates.ImageRequestSync.validate_request(params)},
	 {:get_template, template} when not is_nil(template) <-
	   {:get_template, Templates.get_template(request.template)},
	 {:image_meta, image_meta} <-
	   {:image_meta, append_image_quality_meta(template.project_id, request, stencil_meta)},
	 {:sign_token, {:ok, digest}} <-
	   {:sign_token, generate_digest(user.id, template.project_id)},
	 {:render_template, {:ok, image}} <-
	   {:render_template,
		Templates.render_template(template.id, user.id, request, image_meta)},
	 {:image_service, {:ok, image}} <-
	   {:image_service, Stencil.LambdaGenerateImage.generate_image_sync(image.id, digest)} do

	# do something
	
else
  error -> # handle error
end

Let's compare this with the refactored version (I'll explain everything below).

request =
  Builder.new()
  |> Builder.with_sync()
  |> set_request(params)
  |> Builder.when_valid(&set_template/1)
  |> Builder.when_valid(&set_meta/1)
  |> Builder.when_valid(&set_digest(&1, user.id))
  |> Builder.when_valid(&build_template(&1, user.id))

if Builder.is_valid?(request) do
  case ImageGenerations.generate(request) do
	# do someting
  end
else
  # handle error
  handle_build_errors(request.error)
end

Look how clean it looks - easy on the eyes and read like a sentence.

Constructing a common data structure

The first step to a builder pattern is identifying the common structure that we can group together inside this data structure. If you're using with in the first place, the chance is high that they are all related and can be represented as a single record.

Ours look like this.

defmodule Stencil.ImageGeneration.ImageRequestBuilder do
  defstruct [:request, :template, :meta, :digest, :image, :sync, :error]

  @typep request :: %Stencil.Templates.ImageRequest{}
  @type t :: %__MODULE__{
          request: request(),
          template: %Stencil.Templates.Template{},
          meta: any(),
          digest: String.t(),
          image: %Stencil.Templates.Image{},
          sync: boolean(),
          error: nil | %Stencil.ImageGenerationError{}
        }

  def new do
    %__MODULE__{
      request: nil,
      template: nil,
      meta: nil,
      digest: nil,
      image: nil,
      sync: false,
      error: nil
    }
  end

  def with_sync(struct), do: %{struct | sync: true}

  def is_valid?(%__MODULE__{error: err}) when is_nil(err), do: true
  def is_valid?(%__MODULE__{error: _}), do: false

  def when_valid(%__MODULE__{error: err} = struct, func) when is_nil(err), do: func.(struct)
  def when_valid(%__MODULE__{error: _} = struct, _), do: struct

  def set_error(struct, err), do: %{struct | error: err}
  def set_request(struct, req), do: %{struct | request: req, error: nil}
  def set_template(struct, template), do: %{struct | template: template, error: nil}
  def set_meta(struct, meta), do: %{struct | meta: meta, error: nil}
  def set_digest(struct, digest), do: %{struct | digest: digest, error: nil}
  def set_image(struct, image), do: %{struct | image: image, error: nil}
end

We wrote several helper functions to set the fields, so we can compose these functions with |> operator.

Another interesting helper function is when_valid/2. The purpose of this function is to short-circuit the whole pipeline when we detect an error, that is, it returned the existing record as is. If you're familiar with Haskell, this is the bind function (>>=).

Putting everything together

To put everything together, we can write a few smaller functions that accept this record and do what is needed based on this record. On success, it will set the result to the appropriate field and returned the modified record. On error, it will set the error field with the appropriate error.

When composed with when_valid/2, it will only continue with the next processing if there's no error.

request =
  Builder.new()
  |> Builder.with_sync()
  |> set_request(params)
  |> Builder.when_valid(&set_template/1)
  |> Builder.when_valid(&set_meta/1)
  |> Builder.when_valid(&set_digest(&1, user.id))
  |> Builder.when_valid(&build_template(&1, user.id))

if Builder.is_valid?(request) do
  case ImageGenerations.generate(request) do
	# do someting
  end
else
  # handle error
  handle_build_errors(request.error)
end

The helper functions are written as a function that accepts the record as the first argument so we can pipe them elegantly.

defp set_request(struct, params) do
  case Stencil.Templates.ImageRequestAsync.validate_request(params) do
    {:error, err} -> struct |> Builder.set_error(Error.invalid_request_error(err))
    {:ok, request} -> struct |> Builder.set_request(request)
  end
end

defp set_template(struct) do
  case Templates.get_template(struct.request.template) do
    nil -> struct |> Builder.set_error(Error.template_not_found_error())
    template -> struct |> Builder.set_template(template)
  end
end

This pattern is very easy to understand. I hope you find this useful.