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

Steve Pallen · Accepted Answer · 2017-05-13 00:56:31Z

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.

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)
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
style Use lots of single use variables. This is typically a sign that the pipe |> operator may clean up the code.
style Use logs of conditional statements where multi-clause function heads with pattern matching and guards may make the code cleaner, with less nesting

Hello Steve, thank you very much, I have some questions regarding this solution that I hope you can clear out. 1st. Do I have to use a Changeset even if I don't want a validation? Because when I insert a value the user doesn't input any information, I'm getting it from a struct attached to the user automatically, the only thing the user does is call the command. 2nd. I've read about with before but honestly I didn't quite understand it so I didn't know it could be used in this case, does it work like a nest case statement or something else? I will look further into it and... — Aguxez
– Aguxez, Commented May 13, 2017 at 0:31
... 3rd. I don't actually care (in this case) for the validation errors on Repo.insert, because how I said, I'm not validating any data or I may be wrong? So, should I explicitly check the changeset returned with {:error, changeset} when I'm validating the data? Again, thanks for the help. — Aguxez
– Aguxez, Commented May 13, 2017 at 0:33
No, you don't need to pass Repo.insert a changeset. Your code will work fine. If you don't care about validation errors then you don't need to check. If you do care about validation errors, then changeset.errors will give you the list. Finally, when I first started using the with operator, I found it a little confusing and hard to remember. However, after using it for a while, its very easy to use and a very nice tool to reduce nesting and handling errors in the middle of a pipeline. — Steve Pallen
– Steve Pallen, Commented May 13, 2017 at 0:44

Stack Exchange Network

Elixir - Points bot using Ecto

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Elixir - Points bot using Ecto

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions