I have a function with a condition:
def iteraterefs(divisor, number, table, out, i) when number >= divisor do
number = number - divisor
out = out <> elem(table, i)
end
However it seems that the compiler does not like the condition when number >= divisor do, is there something about reuse of function variables I am missing from the documentation?
I am using the Exercism practice platform which attempts to test the code
** (FunctionClauseError) no function clause matching in RomanNumerals.iteraterefs/5
...
Attempted function clauses (showing 1 out of 1):
def iteraterefs(divisor, number, table, out, i) when number >= divisor
In the above number >= divisor has red text highlighting. The function is being called in a Enum.each like this:
Enum.each(refs, fn(divisor) ->
iteraterefs(divisor, number, table, out, i)
i = i + 1
end)
And is defined as:
def iteraterefs(divisor, number, table, out, i) when number >= divisor do
number = number - divisor
out = out <> elem(table, i)
end
FunctionClauseError is what I’d expect from iteraterefs if it was called with number < divisor. You likely need to define it for those inputs as well to pass Exercism’s tests.
General note: this isn’t going to do what you want. i = i + 1 rebinds the name i inside the do / end block but that value doesn’t escape or even make it to the next iteration.
Same thing for out = out <> elem(table, i) inside iteraterefs; the name out is bound to a new name but then the scope ends immediately.
My recommendation would be to forget completely about Enum.each for a little while; it is almost never the right solution in Elixir.
Isn’t the solution to variable scope to pin the variables?
Pinning variables only performs a match, it doesn’t create a binding. You aren’t actually doing any pinning in your example so I’m not quite sure how you are picturing it would work but, for example, this doesn’t work:
foo = 1
^foo = 1 + 1 # This is a match error
^foo = 1 + 1 gets expanded to 1 = 2 which, of course, does not match.
If you want to accumulate a variable the most basic ways to are either recursion or Enum.reduce.
I was imagining something like foo = ^foo + 1 which would reference the variable in the outer scope. Or is foo destroyed when it leave scope?
Yes, it is. It’s a bit confusing terminology-wise since we do say we can re-bind variables but really it might be easier to think of as you are only reusing the names. It doesn’t affect any outer scopes.
The pin operator can only be used on the lefthand side of a match. The only reason it exists is due to being able to reuse variable names. In Erlang, this isn’t a problem since you are not allow to re-use them.
# Elixir
foo = 1
^foo = 1 # match
foo = 2 # foo is now 2!
% Erlang
Foo = 1.
Foo = 1. % match
Foo = 2. % match error!
It was my reading of those examples from the docs to interpret the ^ as something like a “type/value checked deference”.
Forging ahead, ignoring what I was told about the scope of out, I’ve restructured my code in a recursive manner.
def iteraterefs(number, refs, table, out, i) when number > 0 do
reducenumber(elem(refs, i), number, elem(table, i), out)
iteraterefs(number, refs, table, out, i + 1)
end
def reducenumber(divisor, number, symbol, out) when number >= divisor do
number = number - divisor
out = out <> symbol
end
The compiler parses iteraterefs and produces:
** (ArgumentError) errors were found at the given arguments:
* 2nd argument: not a tuple
Is this an interaction with the elem(refs, i) call? Here is the refs I constructed by hand:
refs = [1000, 900, 500, 400, 100, 90, 50, 40, 10, 9, 5, 4, 1]
Sorry, my example may have been a bit confusing. The out = out <> symbol isn’t doing anything other than assigning out <> symbol to out then throwing it away. You can rebind variable from outer scope.
foo = "bar"
if true do
foo = "bazzzz"
end
foo # This is still "bar"
You can reference an outer scope, but you can’t re-bind.
This I understand from your explanation I was just attempting to address getting the function iteraterefs to compile with inputs. I’ll worry about variable assignment after that.
Well, maybe I attempt both at once, but I can’t get away from that error.
def iteraterefs(number, refs, table, out, i) when number > 0 do
a = if number >= elem(refs, i) do
reducenumber(elem(refs, i), number, elem(table, i), out)
end
out = if elem(a,1) > 0 do
iteraterefs(elem(a, 0), refs, table, elem(a, 1), i + 1)
end
out
end
def reducenumber(divisor, number, symbol, out) when number >= divisor do
number = number - divisor
out = out <> symbol
{number, out}
end
Still it returns a parsing error on iteraterefs:
** (ArgumentError) errors were found at the given arguments:
* 2nd argument: not a tuple
The a will be nil if the condition is not satisfied. You should include an explicit else clause if that is not what you want.
I’m not quite sure which line is doing that. I was kind of blowing past some of your code and realize you have out <> symbol. Assuming (oops sorry, that’s my old Ruby brain talking there)symbol is a symbol, the <> operator only works on strings.
Which line is it failing on? It wouldn’t be elem because the 2nd argument of elem is an integer index.
Speaking of elem, I like to generally stay away from it in favour of pattern matching as it’s much clearer.
Instead of:
tuple = {1, 2}
one = elem(tuple, 0)
two = elem(tuple, 1)
you can simply do:
tuple = {1, 2}
{one, two} = tuple
elem is basically just useful in pipelines (even then I personally don’t like to use it).
It’s erroring on the function signature def iteraterefs(number, refs, table, out, i) when number > 0 do.
IMO this thread is getting a bit too micro, you are kind of posting an error after error and I think you should step back and just post your entire module source code, and state the end goal.
It also sounds like you haven’t practiced Elixir enough if lack of mutability and the lexical scope are still surprising for you. Exercism requires some understanding of the language’s constructs. Without that you’ll just be crashing into one error after another, as it seems it is happening currently.
I went away, far up into the mountains which are the Elixir documents, I learned many things and when I returned to take on the challenge again, it neatly folded before me like the recursion I used to solve it:
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table {"M", "CM", "D", "CD", "C", "XC", "L", "XL", "X", "IX", "V", "IV", "I"}
@refs {1000, 900, 500, 400, 100, 90, 50, 40, 10, 9, 5, 4, 1}
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @refs, @table, 0, "")
end
defp iterate(num, refs, table, index, out) do
if num == 0 do
out
else
ref = elem(refs, index)
cond do
num >= ref -> sym = elem(table, index)
iterate(num - ref, refs, table, index, out <> sym)
true -> iterate(num, refs, table, index + 1, out)
end
end
end
end
Nicely done, a big improvement! Probably the only other change I’d advocate for is to do this:
defp iterate(0, _refs, _table, _index, out) do
out
end
defp iterate(num, refs, table, index, out) do
ref = elem(refs, index)
cond do
num >= ref ->
sym = elem(table, index)
iterate(num - ref, refs, table, index, out <> sym)
true ->
iterate(num, refs, table, index + 1, out)
end
end
It’s conventional that when you’re doing recursion like this to define the “base case” or “termination case” as its own clause up front, and then you have other clauses after. This is entirely a stylistic thing though, so it’s up to you!
Looks reasonable, though using lists can help avoid tricky off-by-one or off-the-end issues with elem.
As a demonstration of that principle, here are some refactors of the module:
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table {{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}}
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, 0, "")
end
defp iterate(num, table, index, out) do
if num == 0 do
out
else
{sym, ref} = elem(table, index)
cond do
num >= ref ->
iterate(num - ref, table, index, out <> sym)
true ->
iterate(num, table, index + 1, out)
end
end
end
end
This first refactor keeps the tuple-shaped table, but brings the symbols and the values together for readability and future maintainability. Having @table and @refs be different-sized tuples would be Not Good, so combining them together makes that bug impossible.
There’s another place for bugs to hide, though: when we write iterate(num, table, index+1, out), that will try to execute elem(table, index+1) and give:
** (ArgumentError) errors were found at the given arguments:
* 1st argument: out of range
:erlang.element(14, {{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}})
iex:21: RomanNumerals.iterate/4
(this happened in an earlier version when I made a typo)
The root cause is that the recursive call to iterate assumes that a “next” element of table exists, without checking. You could add extra checks to ensure that index < tuple_size(table), but again it’s better to write code that literally can’t run off the end.
First, a very bad refactor that makes things slower. Enum.at is expensive compared to elem, since it needs to traverse the list. This also applies the early-exit cleanup that @benwilson512 suggested.
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, 0, "")
end
defp iterate(0, _, _, out), do: out
defp iterate(num, table, index, out) do
{sym, ref} = Enum.at(table, index)
cond do
num >= ref ->
iterate(num - ref, table, index, out <> sym)
true ->
iterate(num, table, index + 1, out)
end
end
end
This will fail in a slightly different way for an negative input (match error vs argument error) but it still “goes off the end” and crashes. Checking length(table) is expensive (traversing the list again!) so checking is even harder. Why am I telling you to use lists anyways?
Two things come together to make lists powerful here:
a call to iterate will only ever care about index or higher in table
There’s one place in a list that isn’t expensive to access: the first element (aka “the head”). It’s also easy to check for, since only [] doesn’t have a head.
This refactor applies that principle: instead of keeping track of table and index separately, it uses hd and tl to interact with the first element and the rest. This is fast and requires no allocations.
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, "")
end
defp iterate(0, _, out), do: out
defp iterate(num, table, out) do
{sym, ref} = hd(table)
cond do
num >= ref ->
iterate(num - ref, table, out <> sym)
true ->
iterate(num, tl(table), out)
end
end
end
A small cleanup refactor: the pattern of “do something with hd and something else with tl” is so common that it’s usually not written explicitly. This uses pattern-matching to accomplish the same thing as the previous version
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, "")
end
defp iterate(0, _, out), do: out
defp iterate(num, [head | rest] = table, out) do
{sym, ref} = head
cond do
num >= ref ->
iterate(num - ref, table, out <> sym)
true ->
iterate(num, rest, out)
end
end
end
It’s also very common to absorb a binding like {sym, ref} = head into the function head:
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, "")
end
defp iterate(0, _, out), do: out
defp iterate(num, [{sym, ref} | rest] = table, out) do
cond do
num >= ref ->
iterate(num - ref, table, out <> sym)
true ->
iterate(num, rest, out)
end
end
end
This version crashes in yet a different way, so what can we do about it? The error message gives us a clue:
iex(26)> RomanNumerals.numeral(-1)
** (FunctionClauseError) no function clause matching in RomanNumerals.iterate/3
The following arguments were given to RomanNumerals.iterate/3:
# 1
-1
# 2
[]
# 3
""
iex:36: RomanNumerals.iterate/3
Think about what iterate being called with [] for table means: the conversion process has run out of symbols to try. That’s directly representable in code:
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, "")
end
defp iterate(0, _, out), do: out
defp iterate(_, [], _), do: raise("bad number")
defp iterate(num, [{sym, ref} | rest] = table, out) do
cond do
num >= ref ->
iterate(num - ref, table, out <> sym)
true ->
iterate(num, rest, out)
end
end
end
Another not-actually-relevant-here-but-worth-keeping-in-mind tip on performance: <> in a loop (or recursion) should be regarded with suspicion as it can result in lots of short-lived binaries. A common idiom avoids the repeated operations and does them at the end:
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, [])
end
defp iterate(0, _, out), do: out |> Enum.reverse() |> Enum.join()
defp iterate(_, [], _), do: raise("bad number")
defp iterate(num, [{sym, ref} | rest] = table, out) do
cond do
num >= ref ->
iterate(num - ref, table, [sym | out])
true ->
iterate(num, rest, out)
end
end
end
As a final cleanup, the cond with one non-default branch could be replaced with an if - or even a guard! The guard style makes the control structures disappear almost completely:
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, [])
end
defp iterate(0, _, out), do: out |> Enum.reverse() |> Enum.join()
defp iterate(_, [], _), do: raise("bad number")
defp iterate(num, [{sym, ref} | _] = table, out) when num >= ref do
iterate(num - ref, table, [sym | out])
end
defp iterate(num, [_ | rest], out) do
iterate(num, rest, out)
end
end
Apologies for the long post, but I wanted to avoid the usual “the steps between these two versions are OBVIOUS” hand-waving and justify each piece.
There is a lot of really good learning in this post. I had been using List splitting in function signatures but had not thought to further disambiguate those elements using a set. The information on Enum and hd/tl was new as was placing wildcards in a signature. The idea of building a list and joining at the end seems to have a lot of value.
© Copyright Elixir Forum | Terms | Privacy & Cookies
