CODEWITHSUNDEEP
elixirphoenix-framework
skovmand
skovmand

Reputation: 4422

Clean up Elixir/Phoenix map replacement code

Do you have any suggestions to how I can make this cleaner?

Video params is from a form submission, so a map as %{"url" => "https://youtube.com/......", "title" => "Carneval in Rio de Janeiro", ...}

defp make_url_ready_for_embedding(video_params) do
  cond do
    String.contains? video_params["url"], "/watch?v=" ->
      video_params |> Map.put("url", String.replace(video_params["url"], "/watch?v=", "/embed/"))
    String.contains? video_params["url"], "https://vimeo.com" ->
      video_params |> Map.put("url", Regex.replace(~r/([^1-9]+)/, video_params["url"], "https://player.vimeo.com/video/"))
    true ->
      video_params
  end
end

Here is my create method if it is of any use:

def create(conn, %{"video" => video_params}, user) do
    changeset = 
      user
      |> build_assoc(:videos)
      |> Video.changeset(video_params |> make_url_ready_for_embedding)

    case Repo.insert(changeset) do
      {:ok, _video} ->
        conn
        |> put_flash(:info, "Video created successfully.")
        |> redirect(to: video_path(conn, :index))
      {:error, changeset} ->
        render(conn, "new.html", changeset: changeset)
    end
  end

Upvotes: 1

Views: 106

Answers (1)

Gazler
Gazler

Reputation: 84180

The logic make sense, however the lines are quite long.

I would probably go for something like:

defp make_url_ready_for_embedding(%{"url" => url} = video_params) do
  url = cond do
    String.contains?(url, "/watch?v=") ->
      String.replace(url, "/watch?v=", "/embed/")
    String.contains?(url "https://vimeo.com") ->
      Regex.replace(~r/([^1-9]+)/, url, "https://player.vimeo.com/video/")
    true ->
      url
  end
  %{video_params | "url" => url)
end

This is better, however the intention is still not very clear at a glance. I would probably consider using functions to aid with this:

defp make_url_ready_for_embedding(%{"url" => url} = video_params) do
  type = cond do
    youtube_video?(url) -> :youtube
    vimeo_video?(url)   -> :vimeo
    true                -> :unknown
  end
  %{video_params | "url" => transform_url(url, type)}
end

defp youtube_video?(url) do
  String.contains?(url, "/watch?v=")
end

defp vimeo_video?(url) do
  String.contains?(url, "https://vimeo.com")
end

defp transform_url(url, :unknown) do: url
defp transform_url(url, :youtube) do
  String.replace(url, "/watch?v=", "/embed/")
end
defp transform_url(url, :vimeo) do
  Regex.replace(~r/([^1-9]+)/, url, "https://player.vimeo.com/video/")
end

This has the benefit that you can test your transform_url function (make it public and @doc false) to ensure that the url is transformed correctly for each type.

Upvotes: 2

Related Questions