Feedback request - my simple GenServer demo

I recently took a code challenge for a job I applied to. My Elixir experience is limited to one production Phoenix project; but as most of you know, with a framework and a fairly simple project, there’s not a lot of skill required beyond basic language use and understanding how to use the framework correctly. Thus, I had not done anything interesting with BEAM before.

Here was the problem spec:

URLShortener

Create a GenServer that acts as a URLShortener which holds the URLs and IDs in its state (a simple map structure would be OK).

Create the following client functions and corresponding callbacks:

  1. shorten/2 , this takes a name/pid and a URL and returns an id
  2. get/2 , this takes a name/pid and an id and returns the url
  3. flush/1 , this takes a name/pid and flushes the state to an empty state
  4. count/1 , this takes a name/pid and returns a total count of the URLs in the state
  5. get_stats/1 , this takes a name/pid and returns the count of URLs per unique hosts from the urls
  6. get_click_stats/1 , this takes a name/pid and returns the total count of clicks on all urls combined (click count is generated from get/2 )
  7. get_click_stats/2 , this takes a name/pid and id and returns the count of clicks on url with given id (click count is generated from get/2 )

And here was my answer:
https://github.com/michaelteter/shorty

I followed a few examples I found online, and I used Elixir/GenServer documentation. However, the feedback I got was limited to “it’s not how we would have done it”. But as it is with job interviews, it’s quite rare to get details. So in my interest to learn, I would be happy to hear any comments from ElixirForum folks. TIA!

1 Like

Sorry for your job interview…

I have seen some part I would change, but given I don’t know the rule it’s kind of difficult to answer fully.

  • Why start_server? I would use start_link
  • Why use tuple when there is no argument? atom is enough I think
  • Maintaining an id count is painful, if allowed, I would use UUID
  • Why create your regex to parse host when there is URI module
  • I would use cast for flush

I find also difficult to reason about the state you are passing.

As a simple example, I would store a simple tuple by given id, something like {host, url, count}

I don’t mean to have the perfect answer, but what about something like this?

  @impl GenServer
  def handle_call({:shorten, url}, _from, state) do
    host = URI.parse(url).host
    id = UUID.uuid4()
    new_state = Map.put(state, id, {host, url, 0})
    {:reply, id, new_state}
  end

  @impl GenServer
  def handle_call({:get, id}, _from, state) do
    case Map.get(state, id) do
      {host, url, count} ->
        {:reply, url, %{state | id => {host, url, count + 1}}}
      nil ->
        {:reply, nil, state}
    end
  end

  @impl GenServer
  def handle_call(:count, _from, state) do
    {:reply, Enum.count(state), state}
  end

  @impl GenServer
  def handle_call(:get_stats, _from, state) do
    reply = state
    |> Enum.group_by(fn {_k, {url, _, _}} -> url end)
    |> Enum.reduce(%{}, fn {key, list}, acc ->
      Map.put(acc, key, Enum.reduce(list, 0, fn {_k, {_, _, count}}, a -> a + count end))
    end)

    {:reply, reply, state}
  end

  @impl GenServer
  def handle_call(:get_click_stats, _from, state) do
    reply = state
    |> Enum.reduce(0, fn {_key, {_host, _url, count}}, acc -> acc + count end)
    {:reply, reply, state}
  end

  @impl GenServer
  def handle_call({:get_click_stats, id}, _from, state) do
    reply = state
    |> Enum.filter(fn {key, _value} -> key == id end)
    |> Enum.reduce(0, fn {_key, {_host, _url, count}}, acc -> acc + count end)
    {:reply, reply, state}
  end

  @impl GenServer
  def handle_cast(:flush, _state) do
    {:noreply, %{}}
  end
2 Likes

Hi @kokolegorille . Thanks very much for taking the time to review and reply.

To answer your questions:

I chose “start_server” as the name because it feels like a server (Gen"Server"). I’m sure there’s a historical reason for internally calling it start_link, but I figured since I was defining the API I could choose a name that I thought fit better. However, if that’s just not recommended, it’s totally no big deal for me to always just expose a start_link().

I’m not sure I understand the tuple vs atom part. Are you saying that in the case of {:some_atom} as an argument, the { } is not necessary? I think I recall pondering that when I was looking at some examples, and if I remember correctly, I decided to stay with the tuple just because it makes the function signature consistent with other GenServer.call()s.

Regarding maintaining ID count, I thought it was actually the simplest way. Each new URL provided gets its own entry in the state structure, so to generate a new ID we just need to add 1 to the count of existing entries. I assumed that Enum.count() on a hashmap was fast. Generating a UUID would be computationally much more expensive, right?

Regex vs URI module… because I didn’t think to look and discover that there was a URI module :blush: .

Regarding cast for flush, I briefly considered it. But since all flush does is return a new tiny structure, I thought it wasn’t worth the overhead. It would also be more work to test, yes?

I should have documented an example and description of what the state structure would look like. Based on the requirements of this challenge, I could have taken a few different approaches in how I structured the data. In the end, I decided to use a bit more space to make the shortening/lookup fast while also making statistics fetching fast. This resulted in the following structure:

state = %{
  urls_by_id: %{
    1 => %{url: "google.com", clicks: 6},
    2 => %{url: "ycombinator.com", clicks: 2},
    ...
  },
  urls_by_hostname: %{
    "google.com" => [
      "https://www.google.com/maps/place/Portugal/@36.8794697,-27.8416912,5z/data=!3m1!4b1!4m5!3m4!1s0xb32242dbf4226d5:0x2ab84b091c4ef041!8m2!3d39.399872!4d-8.224454",
      "https://www.google.com/doodles/"
    ],
    "ycombinator.com" => ["https://news.ycombinator.com/show"],
    ...
  }
}

I think I understand the code you provided, and from my novice perspective it doesn’t seem vastly different from what I did. Or at least, while there are some differences of approach, I don’t feel like my solution was entirely out of line. After all, I basically learned how to make a small GenServer from reading a couple of different guides written by people who seemed to be experienced Elixir devs.

Thanks again for your time.

start_link is not only for historical reason, it means there is a link created between the parent and child processes, which is not the case with start.

And yes, our code makes about the same things :slight_smile:

For the cast… it’s true I start with call everywhere, but when I really don’t expect any response, I switch for cast.

1 Like