New to Elixir, finished first practice project, and would greatly appreciate feedback/tips for improvement

Questions / Help
, ,
1

I’ve been learning Elixir during my month off between semesters. I started a practice project last week (a basic credit card validator application) that I just finished today. If anyone has some time that they wouldn’t mind sharing with me, I would really appreciate some feedback on my code — things like best practices, idiomatic Elixir code, extraneous functions or steps in algorithms, etc.

I’ve pushed the project to GitHub, which can be found here. The primary outer module (Cardinal) contains a simple driver function. The bulk of the work is done in the Cardinal.Validator and Cardinal.Checksum modules.

There are a few tasks/improvement areas that I know I need to address, which I’ve listed in the readme file. I’m sure that I’m missing a lot of areas for improvement, though; I had to rewrite a handful of things throughout the process as I discovered more and more built-in Elixir functions that made certain tasks much easier. Identifying more of such issues would be fantastic for learning more about Elixir.

Thank you very much to anyone willing to take a look and provide feedback.

2 Likes
2

Hi @rcb, good job, I see you’re catching up fast :crystal_ball::rocket:

A few points:

  • You should try mix format :+1:t3: If you can change your editor to do it automatically on save, it gives you a nice feedback loop. On top of that, credo can also bring you some nice tips on style and code organization.

  • Cardinal.Checksum.double_every_other/2 could be like this instead, what do you think?

  defp double_every_other([], _pos), do: []
  
  defp double_every_other([head|tail], pos) when rem(pos, 2) == 0 do
    [head | double_every_other(tail, pos + 1)]
  end
  
  defp double_every_other([head|tail], pos) when rem(pos, 2) == 1 do
    [head * 2 | double_every_other(tail, pos + 1)]
  end

Note that rem/2 can only be at the function head because it’s also a guard.

(EDIT: maybe just using Enum.with_index/1 and Enum.map/2 could be better than the above)

  • Prefer is_binary/1 than is_bitstring/1 if you want a guard to guarantee it’s a String. Every binary is a bitstring, but not every bitstring is a binary.

Cheers!

1 Like
3

Those look a lot cleaner than mine. The double_every_other() function was one I wrote early on when the program worked a bit differently and I don’t think I really revisited it much except for minor cleanup when the program changed. When I get back on my main machine, I’ll try this implementation out.

I didn’t realize that distinction was there between those two. I’ll change those out too.

Thanks for looking things over and taking the time to provide some guidance – I really appreciate it!

1 Like
4

Hello @rcb
Here is my few cents:

  1. It is a good technique that you pass module with effects (IO) as a dependency, this can allow you having more controlled tests… but not now, because those functions are private and public functions are not configurable, so it is not really makes sense. Just type IO.puts directly. You can test this with ExUnit.CaptureIO. Another disadvantage in using module as a dependency is that there is no compiler checks and you can’t navigate to specific function definition in your editor, so use this approach wisely.
  2. Personally I don’t like function naming like this: Cardinal.CardRepo.get_card_params_for, because in Elixir you can have a bit another order with pipe operator, so it won’t be so “readable” in this case:
params
|> extract_credit_card_number()
|> Cardinal.CardRepo.get_card_params_for()
  1. Your validator will fail with exception on invalid data. Just type Cardinal.Validator.validate_checksum(1).
  2. Here is an example of Luhn algorithm with less code https://rosettacode.org/wiki/Luhn_test_of_credit_card_numbers#Elixir
5

For something like this:

  defp reduce_double_digits(number) when is_bitstring(number) do
    if String.length(number) > 1 do
      digits = String.graphemes(number)
      [d1 | tail] = digits
      [d2 | _] = tail
      String.to_integer(d1) + String.to_integer(d2)
    else
      String.to_integer(number)
    end
  end

where you aren’t going to use digits anywhere again, you can just write:

[d1 | tail] = String.graphemes(number)

When I first started I would have thought that maybe the first version was clearer because I could think, “I’m using this function to return a list, and then splitting that list into its head and tail,” but as I get more accustomed to pattern matching, I read that and think, “wait, what is digits used for? Does it show up somewhere later?” and it is actually less clear in a way. Along the same lines you arguably might even write:

      digits = String.graphemes(number)
      [d1 | tail] = digits
      [d2 | _] = tail

as

      [d1 | [d2 | _ ]] = String.graphemes(number)

or maybe less terse but more straightforward as:

[d1, d2] = 
  number
  |> String.graphemes()
  |> Enum.take(2)

but then you realize that Enum.take/2 gives you the first two elements of the list, or just the list if it has only one element, which leads you to (with the edit from one of the other replies above):

defp reduce_double_digits(number) when is_binary(number) do
  number
  |> String.graphemes()
  |> Enum.take(2)
  |> Enum.map(&String.to_integer/1)
  |> Enum.sum()
end
6

I’d probably write that like:

[d1, d2 | _ ] = String.graphemes(number)
7

Thanks for the advice regarding passing IO to the private function. I’ve removed that and replaced it with regular IO.gets/puts calls. You’re right about the invalid data example too — I overlooked a pretty big error there. Thanks for the heads up on that, along with the link to the Luhn algorithm example and for taking the time to respond. I appreciate the help!

Also, thanks to both @srowley and @axelson for the excellent advice regarding the reduce_double_digits() function. I’ll work on implementing your suggestions too.

Thanks to everyone who took the time to review my code and provide such helpful advice. I really appreciate the help and guidance!

1 Like