Parser refactoring ideas

[patrykwozinski]

Hey, I need suggestions related to refactoring the Parser module which is my first time working on the AST.
https://github.com/patrykwozinski/strong_migrations/blob/master/lib/strong_migrations/parser.ex

Everything is covered in 100% by unit tests, so it should be pretty safe -> tests.

[joaothallis]

What about use guards in similar cases to decrease the amount of function clauses?
Example, from:
https://github.com/patrykwozinski/strong_migrations/blob/248adce6de3a341eb5800cc980583585d806daae/lib/strong_migrations/parser.ex#L34-L40
to:

  defp parse_body([{:@, _, [{option, _, [true]}]} | tail], acc)
       when option in [:disable_ddl_transaction, :disable_migration_lock] do
    parse_body(tail, %{acc | disable_ddl_transaction: true})
  end

[yashin5]

1 - you can use only one map
2 - the @SPEC is not right, should be nice if you create a type t to StrongMigrations.Migration(don't use %StrongMigrations.Migration{}, this make the compile time greater)
3 - personally i like to unify functions that only difference is a parameter or a simply return, so you can use 'when' to do this or create dinamically with Enum.each. i like the Enum.approach

defmodule StrongMigrations.Parser do
  @moduledoc """
  The module responsible for reading an ASTand generating a bucket of the information of migration's content
  """

  alias StrongMigrations.Migration

  @doc """
  Parses given migrations into a struct with information about the content
  """
  @spec parse([StrongMigrations.migration_file()]) :: [StrongMigrations.Migration.t()]
  def parse(migration_files) do
    migration_files
    |> Stream.map(fn file_path ->
      file_path
      |> file_path_to_ast()
      |> analyze_code()
    end)
    |> Enum.to_list()
  end

  defp file_path_to_ast(file_path) do
    {file_path,
     file_path
     |> File.read!()
     |> Code.string_to_quoted!()}
  end

  defp analyze_code({file_path, {:defmodule, _, [_, [do: {:__block__, _, body}]]}}) do
    parse_body(body, Migration.new(file_path))
  end

  defp analyze_code({file_path, _}), do: Migration.new(file_path)

  defp parse_body([{:use, _, _} | tail], acc), do: parse_body(tail, acc)

  Enum.each([:disable_ddl_transaction, :disable_migration_lock], fn key ->
    defp parse_body([{:@, _, [{unquote(key), _, [true]}]} | tail], acc) do
      parse_body(tail, %{acc | unquote(key) => true})
    end
  end)

  defp parse_body([{:def, _, [{_, _, _}, [do: {:safety_assured, _, _}]]} | tail], acc) do
    parse_body(tail, acc)
  end

  Enum.each([:index, :unique_index], fn key ->
    defp parse_body(
           [{:def, _, [_, [do: {:create, _, [{unquote(key), _, [_, _, opts]}]}]]} | tail],
           acc
         ) do
      parse_body(tail, %{acc | create_index_concurrently: Keyword.get(opts, :concurrently, false)})
    end

    defp parse_body([{:def, _, [_, [do: {:create, _, [{unquote(key), _, _}]}]]} | tail], acc) do
      parse_body(tail, %{acc | create_index: true})
    end

    defp parse_complex_body(
           [{:create, _, [{unquote(key), _, [_, _, [concurrently: true]]}]} | tail],
           acc
         ) do
      parse_complex_body(tail, %{acc | create_index_concurrently: true})
    end

    defp parse_complex_body([{:create, _, [{unquote(key), _, [_, _]}]} | tail], acc) do
      parse_complex_body(tail, %{acc | create_index: true})
    end
  end)

  defp parse_body(
         [{:def, _, [_, [do: {:drop, _, [{:index, _, [_, _, [concurrently: true]]}]}]]} | tail],
         acc
       ) do
    parse_body(tail, %{acc | drop_index_concurrently: true})
  end

  defp parse_body([{:def, _, [_, [do: {:drop, _, [{:index, _, _}]}]]} | tail], acc) do
    parse_body(tail, %{acc | drop_index: true})
  end

  Enum.each([:drop, :drop_if_exists], fn key ->
    defp parse_body([{:def, _, [_, [do: {unquote(key), _, [{:table, _, _}]}]]} | tail], acc) do
      parse_body(tail, %{acc | drop_table: true})
    end
  end)

  defp parse_body(
         [
           {:def, _,
            [_, [do: {:rename, _, [{:table, _, [_table]}, _column_name, [to: _new_column_name]]}]]}
           | tail
         ],
         acc
       ) do
    parse_body(tail, %{acc | rename_column: true})
  end

  defp parse_body(
         [
           {:def, _, [_, [do: {:alter, _, [{:table, _, _}, [do: {:__block__, _, opts}]]}]]} | tail
         ],
         acc
       ) do
    acc = parse_complex_body(opts, acc)

    parse_body(tail, acc)
  end

  Enum.each([:alter, :__block__], fn key ->
    defp parse_body([{:def, _, [_, [do: {unquote(key), _, opts}]]} | tail], acc) do
      acc = parse_complex_body(opts, acc)

      parse_body(tail, acc)
    end
  end)

  defp parse_body([_head | tail], acc) do
    parse_body(tail, acc)
  end

  defp parse_body([], acc), do: acc

  defp parse_complex_body([{:drop, _, [{:index, _, [_, _, [concurrently: true]]}]} | tail], acc) do
    parse_complex_body(tail, %{acc | drop_index_concurrently: true})
  end

  defp parse_complex_body([{:drop, _, [{:index, _, [_, _]}]} | tail], acc) do
    parse_complex_body(tail, %{acc | drop_index: true})
  end

  defp parse_complex_body(
         [{:rename, _, [{:table, _, [_table]}, _column_name, [to: _new_column_name]]} | tail],
         acc
       ) do
    parse_complex_body(tail, %{acc | rename_column: true})
  end

  defp parse_complex_body([[do: {:remove, _, [_column]}] | tail], acc) do
    parse_complex_body(tail, %{acc | remove_column: true})
  end

  defp parse_complex_body([[do: {:remove_if_exists, _, [_column]}] | tail], acc) do
    parse_complex_body(tail, %{acc | remove_column: true})
  end

  defp parse_complex_body([{:remove, _, [_column]} | tail], acc) do
    parse_complex_body(tail, %{acc | remove_column: true})
  end

  defp parse_complex_body([{:remove_if_exists, _, [_column]} | tail], acc) do
    parse_complex_body(tail, %{acc | remove_column: true})
  end

  defp parse_complex_body(
         [{:alter, _, [{:table, _, [_table]}, [do: {:remove, _, [_column]}]]} | tail],
         acc
       ) do
    parse_complex_body(tail, %{acc | remove_column: true})
  end

  defp parse_complex_body(
         [{:alter, _, [{:table, _, [_table]}, [do: {:remove_if_exist, _, [_column]}]]} | tail],
         acc
       ) do
    parse_complex_body(tail, %{acc | remove_column: true})
  end

  Enum.each([:drop, :drop_if_exists], fn key ->
    defp parse_complex_body([{unquote(key), _, [{:table, _, _table_name}]} | tail], acc) do
      parse_complex_body(tail, %{acc | drop_table: true})
    end
  end)

  defp parse_complex_body([_head | tail], acc) do
    parse_complex_body(tail, acc)
  end

  defp parse_complex_body([], acc), do: acc
end

PS: I had a doubt about the code. should create unique_index have the same opt to create a common index? in both cases create_index_concurrently is used