Open RobStallion opened 5 years ago
@RobStallion great questions! (as always!)
All worth having answers to in the README.md
, thanks! ✨
@RobStallion do you feel that your questions have been answered satisfactorily? 💭 if not, please nudge @Danwhy to give a more detailed answer. 😉
@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.
@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.
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
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
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.
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
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 use
d 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)
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
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
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.
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.
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.
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
@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: ...}
@samhstn the way you suggested looks great, so easy to read :smile_cat:
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?
The
get
andget_by
functions seem to just call theirRepo
equivalents. Are they needed?