Avoid redundant database roundtrips

With Ecto.Multi you should be able to combine both of your initial function to the composed one without duplicating code.

Thanks @LostKobrakai
I do a quick check. Is it what you meant?

Multi.new()
|> Multi.run(:validate_coupon, fn _repo, _ ->
  validate_coupon(coupon_id)
end)
|> Multi.run(:increase_counter, fn _repo, _ ->
  increase_counter(coupon_id)
end)
|> Repo.transaction()

I’m searching in the doc but it’s seems like there are still two roundtrips to the databases. Did i miss something?

A multi will not reduce roundtrips, it will just have them in a single transaction.

If You are validating coupon, why pass coupon_id? Just pass a coupon struct. This way, no need to call db.

And You could pattern match on Coupon to be sure You are working with a valid struct.

validate_coupon(%Coupon{} = coupon)

This would put side effect (loading from db) outside the function, making it pure.

And if You return the updated coupon, You could chain…

coupon_id
|> load_from_db()
|> validate_coupon()
|> increase_counter()

IMO passing around IDs is not really “minimal”; a better alternative is to convert those incoming parameters into domain structs as soon as possible in the controller and pass the structs around in the business logic.

Yeah that’s my first thought on this. But then the extra work that the caller has to do (request to db) makes me feel like I’m duplicating a lot of logic.
Anyway, your opinion on the side effect part is something I’ve never thought of. So I think i would go with this kinda kybrid approach

def validate_coupon(%Coupon{} = coupon) do
end

def validate_coupon(coupon_id) do
end

What do you think about this?

I think the previous post from @al2o3cr is a good advice.

Most prefer to have a functional core, free of side effects, as it is easier to test.

That means pushing db calls out of the core.

Anyway, I don’t see the advantage of passing the id, because sooner or later You will use it to make db call.