Branching logic and multi clause

Hi,

I have written my first PhoenixController from scratch and was feeling quite proud of myself until it came time to dealing with branching logic in the action and my sudden realisation of how it should be done.

So the controller is simple. It takes a file upload, sanitizes the file name, saves it to local storage, gets file_stats and enters a record into the database for the upload. This is all just for a proof of concept, so later on I will do better filename collision (rather than :os.system_time), generate thumbnails etc…

However, if saving the file results in an error, then I want to respond appropriately via the connection and the same also applies if I can’t get the file stats. I believe my case statement is correct, but of course even if the file save does result in an :error then it will still continue onto getting the file stats - which of course would not be good.

I have tried using multi clause functions to handle the File.cp return value, which would then call another multi clause function for handling the File.stat return value, however, I then ran into issues of having variables like ‘save_path’ not available in my multi-clause function and it seems wrong to need to pass those down to the multi clause function and would be a pain to refactor later on…

Any thoughts on where I am going wrong? I am not one of those that is adverse to if statements, but I am curious if there is another way of dealing with this.

Here is my controller so far.

defmodule ServerWeb.MediaController do
  use ServerWeb, :controller
  
  alias Server.Playback
  alias Server.Playback.Media

  def create(conn, %{"media" => upload}) do

    extension = Path.extname(upload.filename)
    filename = upload.filename
               |> build_filename
    filename = "#{filename}-#{:os.system_time(:seconds)}#{extension}"

    relative_path = "uploads/original/#{filename}"
    save_path = "./priv/static/#{relative_path}"

    case result = File.cp(upload.path, save_path) do
      {:error, reason} -> respond_with_reason(conn, reason)
      _ -> result
    end

    file_stats = case File.stat(save_path) do
      {:error, reason} -> respond_with_reason(conn, reason)
      {:ok, file_stats} -> file_stats
    end

    attrs = %{"media_id" => Ecto.UUID.generate(), "path" => "", "customer_id" => 0, "filename" => filename, "type" => "image", "bytes" => file_stats.size}

    case Playback.create_media(attrs) do
      {:ok, %Media{} = _media} -> 
        conn
        |> send_resp(200, "")
    end

  end

  defp respond_with_reason(conn, reason) do
    conn
    |> send_resp(200, reason)
  end

  defp build_filename(original_filename) do
    original_filename
    |> Path.rootname
    |> Zarex.sanitize(padding: 20)
    |> String.replace(~r/\s/, "-")
  end

end

And here is the code where I have started to implement multi-clause functions:

defmodule ServerWeb.MediaController do
  use ServerWeb, :controller
  
  alias Server.Playback
  alias Server.Playback.Media

  def create(conn, %{"media" => upload}) do

    extension = Path.extname(upload.filename)
    filename = upload.filename
               |> build_filename
    filename = "#{filename}-#{:os.system_time(:seconds)}#{extension}"

    relative_path = "uploads/original/#{filename}"
    save_path = "./priv/static/#{relative_path}"

    file_saved(conn, File.cp(upload.path, save_path))

  end

  defp file_saved(conn, {:ok}) do

    file_stats = case File.stat(save_path) do
      {:error, reason} -> respond_with_reason(conn, reason)
      {:ok, file_stats} -> file_stats
    end

    attrs = %{"media_id" => Ecto.UUID.generate(), "path" => "", "customer_id" => 0, "filename" => filename, "type" => "image", "bytes" => file_stats.size}

    case Playback.create_media(attrs) do
      {:ok, %Media{} = _media} -> 
        conn
        |> send_resp(200, "")
    end

  end

  defp file_saved(conn, {:error, reason}) do
    respond_with_reason(conn, reason)
  end

  defp respond_with_reason(conn, reason) do
    conn
    |> send_resp(200, reason)
  end

  defp build_filename(original_filename) do
    original_filename
    |> Path.rootname
    |> Zarex.sanitize(padding: 20)
    |> String.replace(~r/\s/, "-")
  end

end

I’d probably do something like:

  def create(conn, %{"media" => upload}) do
    with(
      extension = Path.extname(upload.filename),
      filename = build_filename(upload.filename),
      filename = "#{filename}-#{:os.system_time(:seconds)}#{extension}",
      relative_path = "uploads/original/#{filename}",
      save_path = "./priv/static/#{relative_path}",
      {:ok, _} <- File.cp(upload.path, save_path),
      {:ok, file_stats} <- File.stat(save_path),
      attrs = %{
        "media_id" => Ecto.UUID.generate(),
        "path" => "",
        "customer_id" => 0,
        "filename" => filename,
        "type" => "image",
        "bytes" => file_stats.size
      },
      {:ok, %Media{} = _media} <- Playback.create_media(attrs)
    ) do
      conn
      |> send_resp(200, "")
    else
      {:error, reason} -> respond_with_reason(conn, reason)
    end
  end

Sadly the formatter formats it very poorly, especially when you get multi-line lines (no blank lines, everything compacted, trailing commas on all the expressions hugely annoyingly, and yet not on the last one inconsistently, etc…), but it handles errors well and so forth. I’ve been trying to come up with a syntax for a variant that handles having old values handled in the ‘else’, and I think I can if I default them (in my own custom block syntax of course, which can fix the formatter annoyances in it too).

But yeah with is good, not great, but quite good, especially for these uses.
And yep, still good to separate things out into more functions as well.

I still think a good monad-pipeline would be superior to ‘with’ though… ^.^;
/me really needs to build one sometime as a library…

Its funny you say that overmind…

1 Like

@OvermindDL1 & @jrichocean - Thanks, that makes sense to me… Oh, and Monads now huh!? Ask a question, learn something new! Looks like I have some more reading to do.

I want code to look the way I want it so I just write it that way and then I change reality to make it work :smiley:

Here is my take on this functionality. I’ve separated the web from the business logic. Using helper functions where-ever I think things clutter the natural flow of things.

I am not saying this is the way to do it. It is basically the with expression used by @OvermindDL1 but the code is refactored. I find this way of approaching development helps me getting things into the right place and prevents me from getting to bogged down with details.

Anyway. The code told me it wanted to look like this:

Make the controller be an interface between your user and your business logic. So refactor heavily and only do things in the controller that has to do with this.

def ServerWeb.MediaController do

  def create(conn, %{"media" => upload}) do
     # Only deal with web interface here.
     # In this case you want to upload a file and you only need two things
     # from the connection (upload filename and path)
    case Logic.upload_file(upload.filename, upload.path) do
      {:ok, %Media{}} -> send_resp(conn, 201, "")
      {:error, reason} -> send_resp(conn, 400, reason)
    end
  end
end

Then separate the rest into your “Business Logic” (here called Logic)

defmodule Logic do

  #
  def upload_file(filename, path) do
    # All the other stuff here should be handled by your
    # logic. You don't want your Web Interface handling
    # file copying and writes to the database
    filename = build_filename(filename)
    save_path = Path.join([ Logic.Config.save_dir(),
                            Logic.Config.upload_dir(),
                            filename ])

    with (
      :ok             <- File.cp(path, save_path),
      {:ok, stats}    <- File.stat(save_path),
      {:ok, media}    <- Playback.create_media(%{media_id: Logic.Media.uuid(),
                                                 path: "",
                                                 customer_id: 0,
                                                 filename: filename,
                                                 type: :image,
                                                 bytes: file_stats.size})
    ) do
      {ok, media}
    else
     {:error, reason} -> {:error, reason}
    end
  end
3 Likes

I think it makes sense to separate concerns like this too and it does look much more readable at a controller level.

However, I don’t see why create_media() would sit in the upload_file() function call and not back inside the controller action. I think its pretty standard to call the context create functions inside the controller actions in Phoenix… In which case I am leaning towards just return file_stats from the upload_file function and doing the rest at controller level…

Thanks for your suggestions.

My thinking is that I want to be able to use the application without phoenix. If it is done in the controller then it is something that has to be done in each other layer that want to interface a user. For example, if you want a cli interface, and an ncurses one, or a WxWidget.

I tend to use the repl a bit here to see if an API feels natural. I might have given the upload_file function a bad name. The intention was perhaps more add_media so that in my repl I could do somthing like this:

> Logic.add_media("superman3.ts", "/tmp")

rather than

> file_size = Logic.upload_file("superman3.ts", "/tmp")
> Playback.create(%{what, are, the, attrs again?, bytes: file_size})
1 Like

I’m not really sure why you’d put all the initial = matches within the with clause. They could just as well be before the with and nothing would change.

I actually follow expede’s repo’s pretty closely (you’ll see my comments all over). :wink:

I’m thinking something more specific to holding multi-state, kind of a monad wrapper of a fake algebraic effects system that is still efficient (I made a horrid hack of one that works well at https://github.com/OvermindDL1/elixir_effects but that was entirely an experiment to see how ‘pure’ of an algebraic effects system I could make). Basically it would replace ‘with’ and actually work well. ^.^

I made a detailed ‘what are monads’ post on these forums somewhere a while back, I think here:

Pure consistency. I want a unified block syntax here and with is only part-way there…

1 Like

You could also use QML: Its declarative, offers an optional WYSIWYG drag and drop designer and is lightyears ahead of spooky stuff like WxWidgets shouder

Projects invest huge amounts of work to get rid of such outdated ui’s, you can avoid writting your software in those in the first place. :wink:

It also helps you to separate ui code and the logic, since it is so by nature.

You can use Elixirscript to convert your code into JavaScript, which is a first class citizen in QML: You can even use JS to interact with the declarative UI code in sophisticated ways.

Its also possible to compile this JS/QML stuff then into native code by Qt’s Quick Compiler.

Overmind can tell you tons more and it might be that somebody is already working on native bindings. It also exist a method to use QML on the web, several ones to be precise: qmlweb is one if it.

So, write once, run everywhere. On the desktop and phones is QML without a single line of code GPU accelerated and automatically scaling to the specific display size.

Much fun ^-^

Spooky? o.O

wxWidgets is very well defined, and it uses native widgets in all places that it can (unlike Qt, though it does render very well).

I’d pick Qt over wxWidgets any time nowadays, but don’t be so down on wxWidgets, it is easily the best GUI toolkit when you want to use pure native rendering.

1 Like

I never ever found a project until, which is quite happy with their decision to choose it.

Well, it might be not that messy as GTK+ while this is surely not that hard.

And yes, it may suck less with non-cross platform code, while such a thing does not make too much sense anyway, imho.