acrolink

acrolink

Code review needed: creating multiple records sharing some values on one API call

With the help and code samples from friends here (@idi527 and others) I have written this function to create multiple book related records. I pass to this function some shared data (current_user and borrower_id) along with a comma separated list of book_ids. I make some custom validations, insert if everything is fine or return error messages if something is wrong. What I receive back at the client side is the result list (as JSON):

  def create_record(%{"user_object" => user, "borrower_id" => borrower_id} = attrs, book_list) do

    book_list = String.split(book_list, ",")

    result = []

    for book_id <- book_list do
      book_id = book_id |> String.to_integer

      alias Ecto.Multi
      record_changeset = Mango.Records.Record.changeset(%Record{user_id: user.id, book_id: book_id}, attrs)

      Multi.new()
      |> Mango.Records.Validators.valid_book_exists_multi(book_id)
      |> Mango.Records.Validators.valid_book_belongs_to_current_user_and_borrower_multi(book_id, user, borrower_id)
      |> Mango.Records.Validators.valid_last_record_closed_multi(book_id)
      |> Multi.insert(:record, record_changeset)
      |> Repo.transaction()
      |> case do
        {:ok, :first_record} -> {:ok}
        {:ok, %{record: record}} -> result = result ++ record
        {:error, :book_belongs_to_current_user_and_borrower, reason, _changes} -> result = result ++ reason
        {:error, :book_exists, reason, _changes} -> result = result ++ reason
        {:error, :last_record_closed, reason, _changes} -> result = result ++ reason
      end
    end
  end

You’re welcome to review it and provide feedback :slight_smile:

Most Liked

idi527

idi527

Does it work?

Note that for ... <- ... is not a for loop, but a list comprehension. You don’t need to do result = [] and result = result ++ reason.

I’d probably try and “invert” the logic and first do the list comprehension, and then all these Multi and Repo.transaction

But without significantly changing the logic of your example, I’d probably write something like:

alias Ecto.Multi
alias Mango.Records.{Record, Validators}

# here I suppose book_list is an already parsed list of integers (book ids)
# so no `book_list = String.split(book_list, ",")` <-- cleaning up and preparing
# the inputs is the responsibility of a "boundary" (like a controller action in phoenix)

@typep multi_result :: {:ok, %Record{} | :first_record} | {:error, operation :: atom, reason :: any, reverted_changes :: map}

@spec create_records(%{(String.t | atom) => term}, %User{}, pos_integer, [pos_integer]) ::
      %{(book_id :: pos_integer) => multi_result}
def create_records(attrs, %User{id: user_id} = user, borrower_id, book_ids) do
  book_ids
  |> Stream.map(fn book_id -> 
    {book_id, Record.changeset(%Record{user_id: user_id, book_id: book_id}, attrs)}
  end)
  |> Stream.map(fn {book_id, record_changeset} ->
    multi_result = 
      Multi.new()
      |> Validators.valid_book_exists_multi(book_id)
      |> Validators.valid_book_belongs_to_current_user_and_borrower_multi(book_id, user, borrower_id)
      |> Validators.valid_last_record_closed_multi(book_id)
      |> Multi.insert(:record, record_changeset)
      |> Repo.transaction()

    {book_id, multi_result}
  end)
  |> Enum.into(%{})
end

It’s also not quite clear to me what this code is supposed to do and whether it should halt at invalid changesets / failed “validators” or continue …

joaquinalcerro

joaquinalcerro

You might want to separate the Multi related code to a different function for code reuse. For example:

def validations(record_changeset, book_id, borrower_id) do
  Multi.new()
      |> Mango.Records.Validators.valid_book_exists_multi(book_id)
      |> Mango.Records.Validators.valid_book_belongs_to_current_user_and_borrower_multi(book_id, user, borrower_id)
      |> Mango.Records.Validators.valid_last_record_closed_multi(book_id)
      |> Multi.insert(:record, record_changeset)
end

def create_record(%{"user_object" => user, "borrower_id" => borrower_id} = attrs, book_list) do
  book_list = String.split(book_list, ",")
  result = []
  for book_id <- book_list do
    book_id = book_id |> String.to_integer
    record_changeset = Mango.Records.Record.changeset(%Record{user_id: user.id, book_id: book_id}, attrs)
    case Repo.transaction(validations(record_changeset, book_id, borrower_id)) do
      {:ok, :first_record} -> {:ok}
      {:ok, %{record: record}} -> result = result ++ record
      {:error, :book_belongs_to_current_user_and_borrower, reason, _changes} -> result = result ++ reason
      {:error, :book_exists, reason, _changes} -> result = result ++ reason
      {:error, :last_record_closed, reason, _changes} -> result = result ++ reason
    end
  end
end

Where Next?

Popular in Questions Top

Tee
can someone please explain to me how Enum.reduce works with maps
New
siddhant3030
Hi, I have to write a raw query for one of my project. But till now I have used ecto queries and don’t have much experience writing raw ...
New
Kurisu
For example for a current url like http://localhost:4000/cosmetic/products?_utf8=✓&amp;query=perfume&amp;page=2, I would like to get: ...
New
vac
Hi, I’m quite new in Elixir and I’m trying to format a string to a PEM format. I have the certificate value like MIIDBTCCAe2...... and I...
New
jerry
Good day to you all. I have been struggling to get a query involving like and ilike to work. Can anyone assist me on this, please? pro...
New
vrod
I am using the Starship cross-shell prompt – it seems pretty nice, but I get some errors: [WARN] - (starship::utils): Executing command ...
New
SoCreat
i’m a new one to elixir which editor can i use vs code? or atom? Thanks! :smiley:
New
Qqwy
Original source of discussion: This topic on the Pragmatic Programmers’ Functional Web Development with Elixir, OTP, and Phoenix forum. ...
New
jason.o
In the code below, if the create action is not set to accept “extra_key” as an input, it errors out with a message shown above. Is there ...
New
vonH
In asking this question I am more interested about the expressiveness of the language itself and less concerned about the availability of...
New

Other popular topics Top

danschultzer
None of the current solutions worked well for me, so I went ahead and built a user management system from scratch. This project took far...
548 29377 241
New
vertexbuffer
Hello, can anybody help here..? I have a list of players and I what to delete an element, but every for loop the list is reverting to ori...
New
gshaw
What is the idiomatic way of matching for not nil in Elixir? E.g., First way: defp halt_if_not_signed_in(conn, signed_in_account) when...
New
josevalim
Hi everyone, One of the features added to Elixir early on to help integration with Erlang code was the idea of overridable function defi...
New
jay1
Why is it that the mnesia database isn’t the most preferred database for use in Elixir/Phoenix?
New
freewebwithme
Using vs code and installed ElixirLS: support and debugger. And I got an error popped up on start up says Failed to run ‘elixir’ comma...
New
shijith.k
I am trying to start a new phoenix project with elixir 1.9, but mix phx.new does not work. It says that ** (Mix) The task "phx.new" could...
New
axelson
This post is a wiki (feel free to hit the edit button near the bottom right of this post to add your own changes!) This post collects co...
239 47930 226
New
hariharasudhan94
I would like to know what is the best IDE for elixir development?
New
hariharasudhan94
Lets say I have map like this fetching from my database %{"_id" =&gt; #BSON.ObjectId&lt;58eb1a7a9ad169198c3dXXXX&gt;, "email" =&gt; ...
New

We're in Beta

About us Mission Statement