Elixir - Points bot using Ecto

Question

I'm making a bot that will be used for "points" tracking and such, like a reward system and I'm using Ecto with postgrex, this is my approach so far, it works but I'm not sure if I'm doing it in an "Elixir", functional way or if I can improve this a bit ^-^

defmodule Cord.Commands do
  ...
  def add_me(msg) do 
    case Repo.get_by(Rewards, user: msg.author.id) do
      # Adds user if it doesn't exist
      nil ->
        case Repo.insert(%Rewards{user: msg.author.id}) do
          {:ok, _struct} ->
            Api.create_message(msg.channel_id, ":white_check_mark:")
          {:error, _changeset} ->
            Api.create_message(msg.channel_id, ":x:")
        end
      _ ->
        Api.create_message(msg.channel_id, "You're already registered")
    end
  end

  # Maps to each id in the list and updates each one
  def thanks(msg) do
    Enum.map msg.mentions, fn user ->
      repo_user = Repo.get_by(Rewards, user: user.id)
      points = repo_user.quantity
      repo_user = Ecto.Changeset.change(repo_user, quantity: points + 1)
      Repo.update!(repo_user)
      Api.create_message(msg.channel_id, "#{msg.author.username} gave a :cookie: to #{user.username}")
    end    
  end

  def not_found(_), do: IO.inspect :ignore
end

Answer 1

Here are a few suggestions...

You could DRY up your code in add_me by returning the string and then making the API call in one place. Also, you should call Repo.insert/1 with a changeset. This will ensure your validation is checked. You can also remove the nesting with the with special form. Its a really handy function that I recommend you learn if haven't already.

  def add_me(msg) do 
    message = 
      with nil <- Repo.get_by(Rewards, user: msg.author.id),
           changeset <- Rewards.changeset(%Rewards{}, %{user: msg.author.id}),
           {:ok, _} <- Repo.insert(changeset) do 
        ":white_check_mark:"
      else
        {:error, _} -> ":x:"
        _ -> "You're already registered"
      end
    Api.create_message(msg.channel_id, message)
  end

You can reduce some of the one-time variables in thanks like this:

  def thanks(msg) do
    Enum.map msg.mentions, fn user ->
      repo_user = Repo.get_by(Rewards, user: user.id)

      repo_user
      |> Ecto.Changeset.change(quantity: repo_user.points + 1)
      |> Repo.update!
      Api.create_message(msg.channel_id, "#{msg.author.username} gave a :cookie: to #{user.username}")
    end
  end    

Finally, it appears that you don't care about the type of error on the Repo.insert. Keep in mind that the changeset returned with {:error, changeset} contains the validation errors.

NOTE:

I see 4 common style / issues with programmers learning Elixir.

1. error Calling a side affect free function without using the return value. Data in Elixir is immutable, so you need need to either capture the results of a function, call it as the last statement so it returned to the caller, or create a side effect in the function (send an event, write to database, file, etc)
2. warning Use a variable outside the scope of the block it was set in. In other words, don't set a variable inside an if, case, or cond, then use it later
3. style Use lots of single use variables. This is typically a sign that the pipe |> operator may clean up the code.
4. style Use logs of conditional statements where multi-clause function heads with pattern matching and guards may make the code cleaner, with less nesting