Input validation confusion!

Questions / Help
1

I’ve just written the module below just as a practice. It converts numbers between different bases. But there is a problem. When converting from a non-decimal basis, the user can give a number with a digit that is greater than or equal to the base, which is invalid. So I want to validate inputs and return an error for such numbers. But I’m not quite sure how I should write it. I tried putting the following line after [h | t] = num in to_decimal:

unless  h < base do
  :invalid
end

But weirdly, the function doesn’t return and continues execution! I also tried the following instead, so that at least I replace the invalid digit with the greatest valid digit:

unless h < base do
  h = base-1
end

This had a weird outcome too. h goes back to its original value after the unless block!
So can anyone help?
Any other comment about the code is welcome.
Thanks

defmodule BaseConvert do
  @symbols %{
    0 => "0",
    1 => "1",
    2 => "2",
    3 => "3",
    4 => "4",
    5 => "5",
    6 => "6",
    7 => "7",
    8 => "8",
    9 => "9",
    10 => "a",
    11 => "b",
    12 => "c",
    13 => "d",
    14 => "e",
    15 => "f",
    16 => "g",
    17 => "h",
    18 => "i",
    19 => "j",
    20 => "k",
    21 => "l",
    22 => "m",
    23 => "n",
    24 => "o",
    25 => "p",
    26 => "q",
    27 => "r",
    28 => "s",
    29 => "t",
    30 => "u",
    31 => "v",
    32 => "w",
    33 => "x",
    34 => "y",
    35 => "z",
    36 => "A",
    37 => "B",
    38 => "C",
    39 => "D",
    40 => "E",
    41 => "F",
    42 => "G",
    43 => "H",
    44 => "I",
    45 => "J",
    46 => "K",
    47 => "L",
    48 => "M",
    49 => "N",
    50 => "O",
    51 => "P",
    52 => "Q",
    53 => "R",
    54 => "S",
    55 => "T",
    56 => "U",
    57 => "V",
    58 => "W",
    59 => "X",
    60 => "Y",
    61 => "Z"
  }

  @digits Map.to_list(@symbols) |> Enum.map(fn {k, v} -> {v, k} end) |> Map.new()

  def convert(num, 10, to) do
    num |> from_decimal(to) |> format()
  end

  def convert(num, from, to) when is_integer(num) do
    num |> to_string() |> convert(from,to)
  end

  def convert(num, from, 10) do
    num |> parse() |> to_decimal(from)
  end

  def convert(num, from, to) do
    num |> parse() |> to_decimal(from) |> from_decimal(to) |> format()
  end

  defp from_decimal(num, base, result \\ []) when is_integer(num) do
    result = [rem(num, base) | result]

    unless num < base do
      from_decimal(div(num, base), base, result)
    else
      result
    end
  end

  defp to_decimal(num, base, result \\ 0) when is_list(num) do
    unless length(num) == 0 do
      [h | t] = num
      result = result + :math.pow(base, length(num) - 1) * h
      to_decimal(t, base, result)
    else
      trunc(result)
    end
  end

  defp format(num) when is_list(num) do
    Enum.map(num, fn x -> @symbols[x] end) |> List.to_string()
  end

  defp parse(num) do
    String.split(num, "")
    |> List.delete_at(-1)
    |> List.delete_at(0)
    |> Enum.map(fn x -> @digits[x] end)
  end
end
2

h inside the unless block cannot modify the value outside, remember that everything is immutable, you could do:

h = case h < base do
 true -> base - 1
 false -> ^h
end

and rebind the value of h in the “upper” scope

3

Apparently that’s wrong:

cannot use ^h outside of match clauses

So I did the following instead and it worked:

h = unless h < base do
    base - 1
  else
    h
end

Thanks

4

First of all:

data = Enum.to_list(?0..?9) ++ Enum.to_list(?a..?z)

result = Enum.reduce(data, %{digits: %{}, symbols: %{}}, fn integer, acc ->
  letter = <<integer::utf8>>
  acc |> put_in([:digits, letter], integer) |> put_in([:symbols, integer], letter)
end)

@digits result.digits
@symbols result.symbols

With this you are iterating data only once and don’t have so huge badly formatted map. :077:

Look that Map.to_list/1 is one iteration, Enum.map/2 is second and Map.new/1 is third.

Secondly:

@digits Map.to_list(@symbols) |> Enum.map(fn {k, v} -> {v, k} end) |> Map.new()
# better
@digits @symbols |> Map.to_list() |> Enum.map(fn {k, v} -> {v, k} end) |> Map.new()

Finally:

unless num < base do
  from_decimal(div(num, base), base, result)
else
  result
end
# better
if num < base do
  result
else
  num |> div(base) |> from_decimal(base, result)
end

I also recommend to take a look at Erlang source if C is not a problem for you. :slight_smile:

You can start from: erts/emulator/beam/big.c:325 (in 22.0-rc.1 release). You can find there source of integer_to_binary/3 in which you can find more inspiration for further speed improvements.

Here is helpful resource:

in which I’m linking some style guides:

1 Like
5

That doesn’t work for me. The way you do it, the integer associated with each symbol is its ascii code. But I want it to be its numeric value, e.g. 10 for a.

Yeah, it may not be that charming, but it does comply with the style guidelines!

Its actually binary.c:325.
Anyway, I’m not using that function, so…I’m not sure what you’re getting at here. Could you be more specific?

Thanks for the comments

6

oops, I did not properly read your code, sorry …
Let me correct that:

data = Enum.to_list(?0..?9) ++ Enum.to_list(?a..?z) ++ Enum.to_list(?A..?Z)
data_with_indexes = Enum.with_index(data)

result = Enum.reduce(data_with_indexes, %{digits: %{}, symbols: %{}}, fn {integer, index}, acc ->
  letter = <<integer::utf8>>
  acc |> put_in([:digits, letter], index) |> put_in([:symbols, index], letter)
end)

Look how simple is change in my code example - it’s always big priority for me. Most configurable and generic solutions are best, because they are easy to change and therefore even with small mistake it’s pretty simply to correct that. Look that I only added one more range and indexes here - most important part of code is not really changed.

Well … remember that every language is designed for its own purposes. Similarly let’s keep kawaii thing in manga/anime. :smiley:

But seriously … while you can have your own opinions styles guides (especially this one which follows Elixir core group) are really good and allows to keep code easy to read. Think that you read your code after (let’s say) year and look that you would probably not remember it well. If that happens having heavy readable code even for your own private projects it’s extremely important rule in my opinion.

yeah, I remember that I was correcting that, but I have checked wrong option (synchronize of selected text with clipboard) in my Plasma 5 clipboard app, so it changed corrected text in clipboard with selected (to replace) text. :slight_smile:

As said it’s just a good source of inspiration i.e. sometimes you just need to stop work on your code and take a look how similar things were solved by others. I believe that there are some optimization things that simplest implementation does not have. Erlang as well as Elixir is maintained by really experienced people. For learning purposes it’s almost like cheating having such a good open source resources and such a waste to don’t take a look on them at least once. :077:

P.S.
No matter what you see - conspiracy theory which says that I’m never sleeping is definitely not true. :wink:

1 Like
7

Characters are just integers, use it:

def digit_value(d) when d in $0..$9, do: d - $0
def digit_value(d) when d in $a..$z, do: d - $a + 10
def digit_value(d) when d in $A..$Z, do: d - $A + 36

Or you could even metaprogramming roughly like this: (untested)

Enum.concat([$0..$9, $a..$z, $A..$Z])
|> Enum.with_index()
|> Enum.each(fn {d, i} ->
  def digit_value(unquote(d)), do: unquote(i)
end)

There are a lot solutions that are nicer than your module attribute and still benefit from compiletime optimisations.

PS: function calls are (usually) faster than map-lookups.

3 Likes