"Elixir code that's a hot mess of confusion and repetition."

lang: elixir·67 lines

//your_submission

ElixirAbomination.txt

defmodule Form do
  @moduledoc """
  A collection of loosely related functions helpful for filling out various forms at the city office.
  """

  @doc """
  Generates a string of a given length.

  This string can be used to fill out a form field that is supposed to have no value.
  Such fields cannot be left empty because a malicious third party could fill them out with false data.
  """
  @spec blanks(n :: non_neg_integer()) :: String.t()
  def blanks(n) do
    String.duplicate("X", n)
  end

  @doc """
  Splits the string into a list of uppercase letters.

  This is needed for form fields that don't offer a single input for the whole string,
  but instead require splitting the string into a predefined number of single-letter inputs.
  """
  @spec letters(word :: String.t()) :: list(String.t())
  def letters(word) do
    word
    |> String.upcase()
    |> String.split("", trim: true)
  end

  @doc """
  Checks if the value has no more than the maximum allowed number of letters.

  This is needed to check that the values of fields do not exceed the maximum allowed length.
  It also tells you by how much the value exceeds the maximum.
  """
  @spec check_length(word :: String.t(), length :: non_neg_integer()) ::
          :ok | {:error, pos_integer()}
  def check_length(word, length) do
    diff = String.length(word) - length

    if diff <= 0 do
      :ok
    else
      {:error, diff}
    end
  end

  @type address_map :: %{street: String.t(), postal_code: String.t(), city: String.t()}
  @type address_tuple :: {street :: String.t(), postal_code :: String.t(), city :: String.t()}
  @type address :: address_map() | address_tuple()

  @doc """
  Formats the address as an uppercase multiline string.
  """
  @spec format_address(address()) :: String.t()
  def format_address(%{street: street, postal_code: postal_code, city: city}) do
    format_address({street, postal_code, city})
  end

  def format_address({street, postal_code, city}) do
    """
    #{String.upcase(street)}
    #{String.upcase(postal_code)} #{String.upcase(city)}
    """
  end
end

//detailed_analysis

Blanks Function

The blanks function is supposed to generate a string of a given length, but it's using 'X' instead of a space, because who needs actual blank spaces, right? And what's with the 'this string can be used to fill out a form field' comment? Is this a joke?

Letters Function

The letters function splits a string into a list of uppercase letters, but it's using String.split instead of String.graphemes, because splitting on an empty string is the most efficient way to do it... said no one ever.

Code Duplication

The format_address function has not one, but two implementations that do the exact same thing, because who needs Don't Repeat Yourself (DRY) principles in their code? It's not like it makes the code harder to maintain or anything.

//suggested_fix

ElixirAbomination_fixed.txt

+defmodule Form do

+ @moduledoc "A collection of functions for filling out forms."

+ def blanks(n), do: String.duplicate(" ", n)

+ def letters(word), do: word |> String.upcase() |> String.graphemes()

+ def check_length(word, length), do: if(String.length(word) <= length, do: :ok, else: {:error, String.length(word) - length})

+ defp format_address(street, postal_code, city), do: "#{String.upcase(street)}\n#{String.upcase(postal_code)} #{String.upcase(city)}"

+ def format_address(%{street: street, postal_code: postal_code, city: city}), do: format_address(street, postal_code, city)

+ def format_address({street, postal_code, city}), do: format_address(street, postal_code, city)

+end