How to optimise this code snippet

Questions / Help
1

I have this code in a Phoenix controller (simplified code):

    `def create(conn, %{"img" => img_params}) do
        img_params =
          if upload = img_params["file"] do
            filePath = 
              for i <- upload do
                a = "/path/to/file/uploaded/#{i.filename}"
                File.cp(i.path, a)
                a                                         #(1)
              end
            %{img_params | "path" => filePath}
          end
        Images.create_img(img_params)
    end`

I have two questions:

  1. The last a looks like an hack but I can’t make it work without it;
  2. As my goal is to pass a list to img_params and I’m using the for-do to populate/manipulate it I don’t know if this way is the best (meaning I would like to be sure that I’m prepend existing list on each iteration and not append it because is much slower).
2

Can you elaborate on what img_params looks like? You’re treating it like it’s a list of uploads but all of your variables are singular, IE upload and filePath.

3

Sorry, I made this work first for a single image upload and then adapted it for multiple image upload and still didn’t change my variable names.
initial img_params (form the client) is:

`%{"file" => [%Plug.Upload{content_type: "image/jpeg", filename: "1.jpg",
    path: "/var/folders/s2/1jjdp6sj267gqt2756hplw9r0000gn/T//plug-1498/multipart-433252-139626-3"},
   %Plug.Upload{content_type: "image/png", filename: "2.png",
    path: "/var/folders/s2/1jjdp6sj267gqt2756hplw9r0000gn/T//plug-1498/multipart-433252-140666-3"}],
  "name" => "test", "path" => "test"}`

and then after transformation is:

`%{"file" => [%Plug.Upload{content_type: "image/jpeg", filename: "1.jpg",
    path: "/var/folders/s2/1jjdp6sj267gqt2756hplw9r0000gn/T//plug-1498/multipart-433252-139626-3"},
   %Plug.Upload{content_type: "image/png", filename: "2.png",
    path: "/var/folders/s2/1jjdp6sj267gqt2756hplw9r0000gn/T//plug-1498/multipart-433252-140666-3"}],
  "name" => "test",
  "path" => ["/Users/paulojaneiro/desktop/code/phoetest/uploaded/1.jpg",
   "/Users/paulojaneiro/desktop/code/phoetest/uploaded/2.png"]}`

Also, next step could be get rid of "file" value in the transformed img_params because I don’t need it anymore.

4

That question #1 is not a hack. The last line of the for comprehension is what it returns into your filePath variable. Regarding #2, in your further example below, it doesn’t look like you’ll be dealing with very long lists, and you may be worrying about premature optimization. for does its job quite well.

That being said, I see 2 main ways to improve the code by removing extra nesting and making better use of pattern matching. The pattern matching makes it clearer what the required format of your img_params looks like. I used Enum.map but you could still use the for comprehension if you like.

def create(_conn, %{"img" => %{"file" => upload} = img_params}) when is_list(upload) do
  file_paths = Enum.map(upload, fn %Plug.Upload{filename: filename, path: path} ->
     a = "/path/to/file/uploaded/#{filename}"
     :ok = File.cp(path, a)
     a
   end)

  img_params
  |> Map.delete("file")
  |> Map.put_new("path", file_paths)
  |> Images.create_img
end
5

Yup this is a good answer.

@PJextra The only final thing I’ll note is that you may want to use something like https://hex.pm/packages/briefly to generate random paths that you copy to. Right now if you upload two files with the same name the last one will clobber all the rest.

1 Like