Would someone be kind enough to code review my phoenix controller action?

Questions / Help
,
1

Hi, I’m just starting out with Elixir/Phoenix and was wondering if someone could give me some feedback on the following code? (I’m working alone so want to avoid picking up bad habits!)

The code handles a user uploading a CSV file to update multiple entries in the db at once, and only if they provide the correct admin password.

  def create(conn, _params, user) do
    admin_password = Application.get_env(:datamo, :admin_password)
    if (conn.params["upload_data"]["admin_password"] == admin_password) do
      datasets = conn.params["upload_data"]["datasets"]

      datasets.path
      |> File.stream!()
      |> CSV.decode(headers: [:name, :description, :file_url, :image_url])
      |> Enum.map(fn dataset_params ->
          user
          |> build_assoc(:datasets)
          |> Dataset.changeset(dataset_params)
         end)
      |> Enum.map(fn changeset ->
          Repo.insert!(changeset)
         end)

      conn
      |> put_flash(:info, "Datasets uploaded successfully.")
      |> redirect(to: dataset_path(conn, :index))
    else
      conn
      |> put_flash(:error, "Admin password incorrect.")
      |> redirect(to: upload_path(conn, :new))
    end
  end

Any feedback on general style/usage would be fantastic!

A few concerns I have myself:

  • No validation on the CSV
  • Should I be using environment variables for the admin password rather than config vars?
  • I should probably have different user permissions for admins rather than a password (authorization)
  • Should I be handling possible errors on the Repo.insert (i.e. not using the ! version)?
  • And similarly, should I be handling possible errors on the File.stream?

Thanks. :slight_smile:

2

I’m no expert, but I think the Elixir way is to pattern match on the params like so:

def create(conn, %{"upload_data" => upload_data} = params, user) do
...
if (upload_data["admin_password"] == admin_password) do
...
datasets = upload_data["datasets"]
... etc.

That cleans it up a bit. I think you may be able to go so far as pattern matching into the nested keys:

def create(conn, %{"upload_data" => %{"admin_password" => admin_password, "datasets" => datasets} = upload_data}} = params, user) do

Then you would be able to have multiple function matches in case you have other requests that does not match this particular structure.

2 Likes
3

And I just realized, if you do the latter nested pattern matching on admin_password, this would clean up your if authorization statement:

def create(conn, _params, _user) do
  conn
  |> put_flash(:error, "Admin password incorrect.")
  |> redirect(to: upload_path(conn, :new))
end

EDIT: This would be the last function clause (and go after the above main function clause), and as such, it would act as a “catch-all” for any unauthorized action attempts.

2 Likes
4

Yes pattern matching on the params is a much better way of doing it, IMO. Pattern Matching grants better separation of concerns and code modularization which will be helpful during debugging and maintenance.

2 Likes
5

Thanks @ibgib and @sashaafm! Pattern matching is quite a new feature to me (it isn’t in the other languages I use) so thanks for the suggestion.

I’ll match on the structure and admin password, separating it into two function matches as you suggest. :slight_smile:

1 Like