How to make guard better?

Questions / Help
1

This is my first line of code since learning this program, can you give a suggestion to be better than the program line below:

defmodule Convert do
  def sped(a) when is_integer(a) and a<=0, do: "a cannot be <= 0"
  def sped(a) when is_integer(a) and a<(999), do: 523 + a * 65536
  def sped(a) when is_integer(a) and a>(999), do: div((a-523),65536)
  def sped(_), do: "Cannot calculate for non-integers"
end
2

Typically we would not include the first and last case. Elixir will crash the process and generate a helpful error message at runtime when the function is called with invalid data.

3 Likes
3

As @lpil said. Alternatively, another idiomatic way you will see in Elixir is to return {:ok, result} in case of success, and {:error, cause} in case of error.

In both cases, the point is to clearly distinguish between successful return values and errors.

Congratulations on your first line of Elixir by the way! :slight_smile:

1 Like
4

from your suggestion, I can see now, so far I just can write the elixir syntax but not the feature of elixir yet. Ok, let me try to find out base on your commend, thanks @Lucaong and @Ipil

5

Personally, I would keep the first and last clause. The only difference is that instead of returning a string, I would raise an exception.
It can sometimes be better to raise a custom error to better understand why an exception is raised.

As mentioned by @lucaong, another method often used is to return {:ok, result} or {:error, cause}.
This gives to the calling function the possibility to do pattern matching in order to know if the function was correctly executed.

Generally, both methods are used and Elixir’s naming convention is that functions that generate errors end with a !.

For your example, the Convert module would contain two functions: sped/1 returning {:ok, result} or {:error, cause} and sped!/1 raising an exception in case of an error.

2 Likes
6

I already try to find out solution base on your clue since this morning, still can’t find it. stuck when to find out how to check is_integer with return {:ok, result}, I thinks this links Is there a better way to handle :ok tuples? is similar issue, but i still can’t solve this. Can you give other clue or directly sample code ? Thx

7

This could be one possible solution, maybe.

defmodule Convert do
  def sped(a) when is_integer(a) and > 0 do
    result = case a < 999 do
      true -> 523 + a * 65536
      false -> div(a - 523, 65536)
    end

    {:ok, result}
  end
  def sped(a) do
    reason = case is_integer(a) do
      true -> :neg_or_zero
      false -> :not_an_integer
    end

    {:error, reason}
  end
end

But in Elixir it is also possible to define a function just for integers greater 0. In this case your can remove the second function and return just result. It depends on what you want.

1 Like
8

Thanks for your solution code and advice.

9

It very much depends on how you wish the function handle not getting an integer argument. The two basic alternatives are either:

  • only return a value when you get an integer argument otherwise generate an exception
  • tag the return value indicating whether it was an integer or not. The common return values for this are {ok,value} or {error,reason}.

You basically have to take your pick. If you don’t have any reasonable values or error reasons to return then the common return values are just ok or error.

2 Likes
10

Just a small style comment, subjective I guess, but I normally avoid to use case when a if can do:

def sped(a) when is_integer(a) and a > 0 do
  result = if a < 999 do
    523 + a * 65536
  else
    div(a - 523, 65536)
  end
  {:ok, result}
end

Or, alternatively, using multiple clauses like @arifan did in the first post:

def sped(a) when is_integer(a) and a <= 0,
  do: {:error, :not_in_range}

def sped(a) when is_integer(a) and a < 999,
  do: {:ok, 523 + (a * 65536)}

def sped(a) when is_integer(a),
  do: {:ok, div((a - 523), 65536)}

def sped(_),
  do: {:error, :not_an_integer}

Note that this solution is slightly different when a = 999 (in the original solution it would have fallen in the last clause, but I assume it was a mistake).

4 Likes
11

Here’s one of the many solutions to your problem.

defmodule Convert do
  def sped!(a) when not is_integer(a), do: raise "cannot calculate for non-integers"
  def sped!(a) when a <= 0, do: raise "a cannot be <= 0"
  def sped!(a) when a < 999, do: 523 + a * 65536
  def sped!(a), do: div(a - 523, 65536)

  def sped(a) when not is_integer(a), do: {:error, :not_an_integer}
  def sped(a) when a <= 0, do: {:error, :neg_or_zero}
  def sped(a), do: {:ok, sped!(a)}
end

You’ll notice that I’ve changed the order of some clauses to avoid repeating guards (is_integer(a)).

As @rvirding said above, you don’t necessarily need both functions, you can just choose to implement one. It depends on how you plan to deal with your errors.

3 Likes
12

Yes, should be a <= 999.

13

sure, I will use one of them, I am very happy with all solutions because this all makes me more understood about elixir/function language. Thanks very much for this all.

14

Reading your advice, it changes my way of thinking about how to make a good program, because so far I only think that the program that I created can run as expected and it’s finished. Thanks very much, this helps me to be good.