Questions on approach taken

[RobStallion]

• Why is everything done using macros? We could achieve the same result by making them regular functions which takes the relevant module as an argument. Is there any benefits of one approach vs the other?

• Is the insert function just allowing the user to skip the step where they call the relevant changeset function? Is this a good idea?

  • When inserting into the database in this way will a user only ever need on changeset function? In a regular Phoenix application there may be multiple changesets depending on what needs to be inserted/updated in the database. If a user needs to change their password for example there could be a changeset that deals specifically with just this. (Just to be clear, I cannot actually think of a situation with this approach where a user might need more than one changeset function but thought it best to raise the point in case I have missed something)

• The get and get_by functions seem to just call their Repo equivalents. Are they needed?

[nelsonic]

@RobStallion great questions! (as always!) All worth having answers to in the README.md, thanks! ✨

[nelsonic]

@RobStallion do you feel that your questions have been answered satisfactorily? 💭 if not, please nudge @Danwhy to give a more detailed answer. 😉

[RobStallion]

@nelsonic @Danwhy Sorry if I am being dense but I haven't actually seen the answers yet. I did look at the module and saw that the get function has been updated but that is the only change I have noticed.

Please let me know if I am missing something.

[nelsonic]

@RobStallion you are not being "dense"; This is an excellent question and one which many other people will have! @Danwhy / @samhstn are best placed to explain the differences/advantages of functions vs. macros in this use-case.

[samhstn]

Macros vs Regular functions

Say we have our AppendOnly module with the following functions

defmodule AppendOnly do
  def insert_logic(changeset) do
    ...
  def update_logic(changeset) do
    ...
  def one_logic(struct) do
    ...
  def all_logic(struct) do
    ...
end

And say we have the following modules with insert, update, one, all functions behaving the same way

defmodule Address do
  def insert(struct), do: struct |> Address.changeset() |> AppendOnly.insert_logic() |> Repo.insert()
  def update(struct), do: struct |> Address.changeset() |> AppendOnly.update_logic() |> Repo.insert()
  def one, do: Address |> AppendOnly.one_logic() |> Repo.one()
  def all, do: Address |> AppendOnly.all_logic() |> Repo.all()
end

defmodule Customer do
  def insert(struct), do: struct |> Customer.changeset() |> AppendOnly.insert_logic() |> Repo.insert()
  def update(struct), do: struct |> Customer.changeset() |> AppendOnly.update_logic() |> Repo.insert()
  def one, do: Customer |> AppendOnly.one_logic() |> Repo.one()
  def all, do: Customer |> AppendOnly.all_logic() |> Repo.all()
end

Where, for example, the Address.insert function would be used in the following way:

Address.insert(%Address{address: "1600 Pennsylvania Avenue NW", state: "Washington"})

Obviously this is WET code and needs to be cleaned up.

When refactoring here, the only thing that needs to remain the same is that the Address and Customer insert, update, one and all functions keep their behaviour

Regular functions refactor approach

As you say @robstallion, one approach is to pass in the module into the AppendOnly insert, update, one, all functions.

(after a refactor of AppendOnly to expose insert, update, ...) this would look something like:

defmodule Address do
  def insert(struct), do: AppendOnly.insert(Address, struct)
  def update(struct), do: AppendOnly.update(Address, struct)
  def one, do: AppendOnly.one(Address)
  def all, do: AppendOnly.all(Address)
end

Change 1

Lets say that we now wanted to add a delete function to all modules following this pattern, we would add

...
def delete(struct),
  do: AppendOnly.delete(<Module>, struct)

to every module you wish to add it to.

and add a delete function to the AppendOnly module.

Change 2

Lets say that we now want to change the behaviour of the insert function but only in the Address module

defmodule Address do
  def insert(struct), do: struct |> something_else() |> Repo.insert
  def update(struct), do: AppendOnly.update(Address, struct)
  def one, do: AppendOnly.one(Address)
  def all, do: AppendOnly.all(Address)
end

Macro refactor approach

Another approach is to construct the AppendOnly module as a macro exposing insert, update, one and all as overridable functions

Once this is configured we could set up the Address module like so:

defmodule Address do
  use AppendOnly
end

One really nice thing about macros is that it can reference the module where it is being used and hence, no need to pass the module name anywhere

We repeat as little code as possible in each module following our AppendOnly pattern (making this approach super DRY)

Change 1

We would now like to add a delete function to all modules following this pattern (these would be those with use AppendOnly)

We would simply add a delete function to our AppendOnly module, this would automatically propagate through to Address and Customer

Change 2

We would now like to override the insert function but only in the Address module.

We would simply add our new insert function to the Address module

defmodule Address do
  use AppendOnly

  def insert(struct), do: struct |> something_else() |> Repo.insert
end

Comparing the two approaches

The initial code setup for a module following the append only pattern is a lot cleaner using macros. One downside of using macros is that it is not very declarative and someone could be confused as to where the insert function is coming from. In the regular functions approach, it is super clear where insert is defined for instance.

Change 1

Adding a delete function using the macro approach is super clean, you only need to update the AppendOnly module and no other file changes are needed, you aren't going to forget to add it anywhere because the changes propagate automatically. When you add a delete function using the regular functions approach you must update every module which follows the append only pattern to include it. When testing in the regular function approach, if you would like to "cover" this new delete function everywhere you would also need to add a test for each delete function in every module, whereas in the macro approach we should only need add one test for the one place delete has been added.

Change 2

The difference here between the two approaches isn't much, there is the same amount of code added, but one difference could be that the macro approach is far less cluttered by other functions which we don't want to override.

In short, I think that the use pattern was intended specifically for this type of abstraction where you have a common set of functions used in different modules.

[mischa-s]

Hello, complete elixir n00b chiming in. Thanks so much for this and all the other great repos on Elixir that you have created.

I tripped over

def update(%__MODULE__{} = item, attrs) do
  item
  |> Map.put(:id, nil)
  |> Map.put(:inserted_at, nil)
  |> Map.put(:updated_at, nil)
  |> __MODULE__.changeset(attrs)
  |> Repo.insert()
end

with the typo

def update(%__MODULE__{} = item, attrs) do
...
  |> Map.put(:insterted_at, nil)
...
end

Finding this typo was very hard because the |>Map.put syntax doesn't raise any compile errors. I went over this with a friend who suggested the following

      def update(%__MODULE__{} = item, attrs) do
        item
        |> struct!(
          id: nil,
          inserted_at: nil,
          updated_at: nil
        )
        |> __MODULE__.changeset(attrs)
        |> Repo.insert()
      end

This did produce much more sensible error messages because it doesn't allow the addition of new properties, but I'm curious if there are reasons that doing each attribute separately in a map.puts makes more sense

[samhstn]

@mischa-s nice typo catch 👍 I think you may be on to a potential code smell, nice work

Another way of updating a key and throw a an error if the key doesn't exist is to use the built in update syntax over using the Map.put function.

So,

item
  |> Map.put(:id, nil)
  |> Map.put(:insterted, nil)
  |> Map.put(:updated_at, nil)
  |> __MODULE__.changeset(attrs)
  |> Repo.insert()

Could be:

%{ item | id: nil, insterted: nil, updated_at: nil }
|> __MODULE__.changeset(attrs)
|> Repo.insert()

The typo would have thrown the following error:

** (KeyError) key :insterted_at not found in: %{id: 1, inserted_at: ...}

[mischa-s]

@samhstn the way you suggested looks great, so easy to read :smile_cat: