How can I improve this code

Questions / Help
, ,
1

I’m working on a module that will return details on a website video. You pass in the video provider and video_id and you’ll get back details on the url, embed_url and thumbnail.

I have 2 public calls

// Supported video providers
iex(30)>  VideoProviders.providers
iex(31)>  [:youtube, :vimeo]


// Fetch the details on a valid video
 iex(32)> VideoProvider.get_src("youtube", "IZvpKhA6t8A")
 iex(33)> %{api: "//gdata.youtube.com/feeds/api/videos/IZvpKhA6t8A",
            embed: "//www.youtube.com/embed/IZvpKhA6t8A", provider: :youtube,
            thumbnail_url: "//img.youtube.com/vi/IZvpKhA6t8A/maxresdefault.jpg",
            video_id: "IZvpKhA6t8A"}

// Return an error if we don't support the provider
 iex(34)> VideoProvider.get_src("YOYOTUBE", "IZvpKhA6t8A")
 iex(35)> %{:error, "provider not supported"}

Looking for Feedback
What I have made works BUT I just don’t know if a better approach exists?
• Is this an acceptable way of using pattern matching?
• Is this an acceptable way of using :ok and :error tuples?
• Any other ways of organizing this?

defmodule VideoProvider do
  def get_src(provider, video_id) do
    provider = String.to_atom(provider)

    if Enum.member?(providers(), provider)  do
      {:ok, get_data(provider, video_id)}
    else
      {:error, "provider not supported"}
    end

  end

  def providers, do: ~w(youtube vimeo)a

  defp api_url({:youtube, video_id}), do:  "//gdata.youtube.com/feeds/api/videos/#{video_id}"
  defp api_url({:vimeo, video_id}), do:  "//vimeo.com/api/oembed.json?url=http%3A//vimeo.com/#{video_id}"

  defp embed_url({:youtube, video_id}), do: "//www.youtube.com/embed/#{video_id}"
  defp embed_url({:vimeo, video_id}), do: "//player.vimeo.com/video/#{video_id}"

  defp thumbnail_url({:youtube, video_id}), do: "//img.youtube.com/vi/#{video_id}/maxresdefault.jpg"
  defp thumbnail_url({:vimeo, video_id}), do: "/vimeo/somewhere/#{video_id}"

  defp get_data(provider, video_id) do
    data = {provider, video_id}
    %{
      api_url: api_url(data),
      embed_url: embed_url(data),
      thumbnail_url: thumbnail_url(data),
      provider: provider,
      video_id: video_id
    }
  end
end
2

Seems like its on the right track, though without more context, it’s hard to give precise feedback. For example, what’s calling get_src? Something exposed to external users or only internal code? I ask because String.to_atom can be risky to call on invalidated-input. Atom’s aren’t garbage collected and if, for example, this was a website with src= being a querystring parameter, someone could keep passing random values (a, aa, aaa, ab, abb, abb, ac, acc, abc, …) and cause the system to run out of memory.

To that end, and also to avoid the Enum.members? O(N) check, I’d add another function:

case get_provider(provider) do
 :invalid -> error
  provider -> ...
end
...

def get_provider("youtube"), do: :youtube
def get_provider("video"), do: :vimeo
def get_provider(_), do: :invalid

Finally, I’d probably generate these functions as it’s a little repetitive if you have more than 2 (or not, your choice). something like:

@providers [
     youtube: [api: "...", embed: "..."],
     vimeo: [api: "....", embed: "..."],
  ]

  for {name, opts} <- @providers do
    api = opts[:api]
    embed = opts[:embed]
    sname = Atom.to_string(name)
    def provider(unquote(sname)), do: unquote(name)
    def api_url(unquote(name)), do: unquote(api)
    def embed_url(unquote(name)), do: unquote(embed)
  end
  def provider(_), do: :invalid
3

Seems like its on the right track, though without more context, it’s hard to give precise feedback. For example, what’s calling get_src? Something exposed to external users or only internal code? I ask because String.to_atom can be risky to call on invalidated-input. Atom’s aren’t garbage collected

What’s calling VideoProvider.get_src is internal code. I have a phoenix app and I plan to use this module to loop over 400 videos. No query parameters are passed in. But the data is from a DB.

I’m guessing by your statement then don’t use the :atom approach in this context. Stick to strings.

case get_provider(provider) do

I like this approach on the case method. Thanks.