Looking for feedback improving a reducer function

Almah · February 16, 2024, 9:16am

Hey there!

I’m working on a little pet project, a tool to help me evaluate and play around with random tables for roleplaying games. In the analog world these tables have the following shape:

d100 Individual mission
01-05 Bribe or negotiate with
06-10 Bring message to location of
11-15 Capture or arrest
16-20 Conceal/smuggle
21-25 Deliver/transport
26-30 Discover identity of
31-35 Distract, decoy, or deceive
36-40 Entrap or sting
41-45 Extort from
46-50 Find missing
51-55 Get help from
56-60 Guard/protect
61-65 Incriminate or frame
66-70 Kidnap
71-75 Kill/assassinate
76-80 Rescue
81-85 Sabotage efforts of
86-90 Spy upon
91-95 Steal item from
96-00 Waylay

I’d roll two ten-sided dice (one as tens, one as ones) and get some inspiration for my group’s next mission. Now I’ve done something similar to this in a Kotlin project, but since I’m on the elixir hype train I’m rewriting it in elixir

The output structure is:

Everything works and I can roll on my table and either get a value or reference tuple.

A value tuple is when the result of rolling on a random table is a simple value (all the entries above are simple values).
A reference tuple is when the table references another table to be rolled on. E.g. the “Individual subject or target” table refers to another table 1-8 and I store that reference.

My only beef is that my code, especially parse_collection looks pretty ugly with all of the error handling. I think it could be rewritten to be shorter and prettier, but that’s where my elixir-foo is failing me I’d appreciate any and all feedback, don’t be afraid to be critical if you must.

You should be able to copy and paste this into your own project and run it. You have to comment out the reference to and invocation of RandomTableCollection, but that part’s not relevant for this discussion anyway Let me know if you have any questions and I appreciate the help!

defmodule TheAdventuringDay.Component.RandomTableParsing.DomainService.RandomTableParser do
  @moduledoc """
  TODO
  """

  alias TheAdventuringDay.Component.RandomTable.Domain.RandomTableCollection

  @doc """
  Creates a random table collection from the contents of a file. The file must be formatted into table blocks

  d2 A beggar's request
  1. A new pet to replace the one they lost.
  2. A million gold pieces!

  d2 Random loot
  1. A jar of dirt.
  2. An army of orcs, oops!
  
  A table is composed of two parts:

  - A header with a die size and description (will be converted to the table_name)
  - A number of results equal to the header's dX value. These may reference other tables in this file by using ##, e.g. #patron_or_target#
  """
  def parse(collection_name) do
    with {:ok, file_path} <- file_path("#{collection_name}.txt"),
         {:ok, table_collection} <- parse_collection(file_path) do

      RandomTableCollection.new(table_collection, collection_name)
    end
  end

  defp file_path(filename) do
    file_path =
      "."
      |> Path.expand()
      |> Path.join("data/unstructured_random_tables/#{filename}")

    if File.exists?(file_path) do
      {:ok, file_path}
    else
      {:error, :file_not_found}
    end
  end

  # dev note: I am reading each table block from file by using two states:
  # :read_header means that the next line is expected to be a header "d100 Random loot"
  #   on successfully parsing the header it goes to the next state :read_entries
  # 
  # It continues attempting to read entries until a blank line is found, in which case the 
  # random_table is completed. A new table entry is created to read new table information.
  # The state is then reset to :read_header.
  defp parse_collection(file_path) do
    table_contents =
      file_path
      |> File.stream!()
      |> Enum.reduce_while([%{state: :read_header}], fn line, acc ->
        curr_table = acc |> hd

        case curr_table.state do
          :read_header ->
            case read_header(line) do
              {:ok, table_range, table_name} ->
                updated_acc =
                  acc |> List.update_at(0, fn table ->
                    table
                    |> Map.put(:table_name, table_name)
                    |> Map.put(:table_range, table_range)
                    |> Map.put(:state, :read_entries)
                  end)

                  {:cont, updated_acc}

              error -> {:halt, error}
            end

          :read_entries ->
            case read_entries(line) do
              {:ok, :no_more_entries} ->
                {:cont, [%{state: :read_header} | acc]}

              {:ok, entries} ->
                updated_acc =
                 acc |> List.update_at(0, fn table ->
                    table
                    |> Map.update(:entries, entries, &(entries ++ &1))
                  end)

                {:cont, updated_acc}

              error -> {:halt, error}
            end
        end
      end)

    case validate_table(table_contents) do
      :ok -> {:ok, table_contents |> Map.new(fn %{table_name: table_name, entries: entries} -> {table_name, entries} end)}
      errors -> errors
    end
  end

  defp read_header(header_line) do
    header_match =
      ~r/d(\d+|%) (.+)/i
      |> Regex.scan(header_line)
      |> List.flatten()

    case header_match do
      [_all_match, "%", table_header] -> {:ok, 100, to_table_name(table_header)}
      [_all_match, range_match, table_header] -> {:ok, String.to_integer(range_match), to_table_name(table_header)}
      [] -> {:error, :no_header_match}
    end
  end

  defp to_table_name(table_header) do
    table_header
    |> String.trim()
    |> String.downcase()
    |> String.replace(" ", "_")
  end

  defp read_entries(line) do
    empty_line_regex = ~r/^\n$/i
    entry_regexes = [
      ~r/(\d+)-(\d+) (.+)/i, # Match range results, i.e. 01-05 <result>
      ~r/\d+ (.+)/i          # Match single result, i.e. 01 <result>
    ]

    if String.match?(line, empty_line_regex) do
        {:ok, :no_more_entries}
    else
      matching_regex =
        entry_regexes
        |> Enum.find(fn regex -> String.match?(line, regex) end)

      if matching_regex != nil do
        matching_regex
        |> Regex.scan(line)
        |> List.flatten()
        |> to_entry_list()
      else
        {:error, :could_not_parse_entries}
      end
    end
  end

  defp to_entry_list([match, range_begin, "00", entry]), do: to_entry_list([match, range_begin, "100", entry])

  defp to_entry_list([_, range_begin, range_end, entry]) do
    range_begin = String.to_integer(range_begin)
    range_end = String.to_integer(range_end)
    table_entry = to_table_entry(entry)

    {:ok, (for _ <- range_begin..range_end, do: table_entry)}
  end

  defp to_entry_list([_, entry]), do: {:ok, [to_table_entry(entry)]}

  defp to_entry_list([]), do: {:error, :could_not_parse_entry}

  defp to_table_entry(entry) do
    reference_regex = ~r/^#[^#]+#/i

    if String.match?(entry, reference_regex) do
      {:reference, String.trim(entry, "#")}
    else
      {:value, entry}
    end
  end

  # Double-check that, if a table is supposed to have 100 entries (d100), then it contains exactly 100 entries, and so on.
  defp validate_table(table_contents) do
    validation_errors =
      table_contents
      |> Enum.reduce([], fn %{table_range: table_range, entries: entries, table_name: table_name}, acc ->
        if table_range != length(entries) do
          [{:error, "#{table_name} should have #{table_range} entries, but was #{length(entries)}"} | acc]
        else
          acc
        end
      end)

    if length(validation_errors) == 0 do
      :ok
    else
      {:error, validation_errors}
    end
  end
end

sodapopcan · February 16, 2024, 2:49pm

As a small suggestion, you could tighten up your reduce_while to something like this:

|> Enum.reduce_while([%{state: :read_header}],
  fn line, [%{state: :reader_header} | _] = acc ->
    # ...
  fn line, [%{state: :read_entries} | _] = acc ->
    # ...

al2o3cr · February 16, 2024, 3:01pm

Some notes in no particular order:

A common pattern I end up writing a lot in this position is |> Stream.map(&String.trim/1), since the rest of the code doesn’t usually care about line-endings.

Minor nitpick: hd(acc) is easier to read and shorter than the pipe version

Almah:

                updated_acc =
                  acc |> List.update_at(0, fn table ->
                    table
                    |> Map.put(:table_name, table_name)
                    |> Map.put(:table_range, table_range)
                    |> Map.put(:state, :read_entries)
                  end)

The first element of the list in the accumulator is clearly special. Consider keeping it as a separate piece of state in the reduce_while:

|> Enum.reduce_while({%{state: :read_header}, []}, fn line, {curr_table, acc} ->

IMO the core thing driving complexity in that reduce_while is that it has two overlapping responsibilities:

splitting the input into chunks at empty lines
parsing a table from each chunk of lines

That first part could be written:

File.stream!(...)
|> Stream.map(&String.trim/1)
|> Stream.chunk_while(
  [],
  fn
    "", [] -> {:cont, []} # NOTE: omit this case if you want repeated blank lines to appear in the output
    "", acc -> {:cont, Enum.reverse(acc), []}
    line, acc -> {:cont, [line | acc]}
  end,
  fn
    [] -> {:cont, []}
    acc -> {:cont, Enum.reverse(acc), []}
  end
)

Almah · February 16, 2024, 7:08pm

Thank you so much for the feedback! chunk_while is the function I was searching for, but couldn’t find I also totally forgot that elixir can pattern match on functions so you can write two or three of them like both of you did.

My new parsing function is much smaller and cleaner

  defp parse_collection(file_path) do
    table_contents =
      file_path
      |> File.stream!()
      |> Stream.map(&String.trim/1)
      |> Stream.chunk_while(
        [],
        fn
          "", acc -> {:cont, Enum.reverse(acc), []}
          line, acc -> {:cont, [line | acc]}
        end,
        fn
          [] -> {:cont, []}
          acc -> {:cont, Enum.reverse(acc), []}
        end)
      |> Enum.map(fn [header | entries] ->
        with {:ok, table_range, table_name} <- read_header(header),
             {:ok, entries} <- read_table_entries(entries) do
          %{
            table_name: table_name,
            table_range: table_range,
            entries: entries
          }
        end
      end)

    case validate_table(table_contents) do
      :ok -> {:ok, table_contents |> Map.new(fn %{table_name: table_name, entries: entries} -> {table_name, entries} end)}
      errors -> errors
    end
  end