Best Practice to get rid of if...else blocks?

Rukenshia

I am used to early returns using the “return” keyword many languages provide.
As I am currently trying to learn elixir, I found it really hard to get a proper structure in my code and avoid if…else blocks which I have to nest very deep.

As an example, I wrote a small bot for Discord (don’t do it by the way, the discord_ex library doesn’t seem to work properly) and have to check several things when I receive a message.

As an example, here is one handler.

defp handle_command({ "msg", "remove-trigger", cmd }, { payload, state }) do
    if payload.data["author"]["id"] == 1234 do
      shortname = String.split(cmd, " ") |> List.first
      if !Messages.has shortname do
        send_message "unknown Message", { payload, state }
      else
        info = Messages.get(shortname)
        trigger = String.replace_leading(cmd, "#{shortname} ", "")
        if Enum.member?(info[:trigger], trigger) do
          Messages.remove_trigger shortname, trigger
          save_messages
        end

        send_message "trigger #{trigger} removed from #{shortname}", {payload, state}
      end
    end
  end

I am pretty sure that this can be way improved - what’s the proper design pattern for that in Elixir, how do I escape the if-block hell?

5 comments

#best-practices #design-problem

2 5193 5

2016-11-25 22:30:07 UTC

Showing Posts 1 to 5

Show Best Posts
Show All (oldest first)
Show All (newest first)

dardub

I’m new to elixir as well and still figuring this out. But one thing you can do is break this up into more functions and take advantage of pattern matching.

For instance you can get rid of the first if statement by pattern matching the handle_command function:

defp handle_command({ "msg", "remove-trigger", cmd }, { %{data: %{"author" => %{"id" => 1234}}}, state }) do

Post #1

ibgib

In addition to the pattern matching that @dardub mentioned (), I might change the structure of the Messages module’s functions to a tagged format so you aren’t using the if..then for control flow.

So your Messages.get/1 would return either {:ok, info} or {:error, :not_found} (or more generally {:error, reason} (EDIT: And you wouldn’t even use the if has/1 in your example). To me, this would make it more of an expression approach using a case or with statement with a more understandable “error” workflow.

Post #2

atimberlake

You can pattern match the handle_command function to drop the first if

defp handle_command({"msg", "remove-trigger", cmd}, {%{data: %{"author" => %{"id" => 1234}}}, state}) do
   # handle author 1234  
end
defp handle_command({"msg", "remove-trigger", cmd}, {payload, state}) do
  # No-op because you have no else
end

For the second if I would drop the call to Message.has/1 (assuming Message.get/1 returns nil for no message)

shortname = String.split(cmd, " ") |> List.first
do_send_message(Messages.get(shortname))

Then have a function deal with the presence or lack of message

defp do_send_message(nil) do
  send_message "unknown Message", {payload, state}
end
defp do_send_message(info) do
  trigger = …
end

Keep going by creating functions that handle each situation and try to come up with names that reveal the intent of each step (do_send_message is a terrible function name but because naming is hard, you get to solve that )

Post #3

josevalim

Creator of Elixir

To add to this, at some point the community, even Elixir codebase, settled on do_send_message as a convention but today we see it as a bad practice. We are trying our best to not use do_ prefixes in Elixir and use proper function names for them, even if it ends up being the original name with extra information, for example, check_shortname_and_send_message.

Post #4

Rukenshia

Thanks a lot guys, this really helped me. It’s really easy for me to forget how powerful the pattern matching just is. I know Rust’s pattern matching which is pretty cool already, but this is way more powerful.

Post #5

— All posts loaded —