Allow for alternative erlang solutions in `captains-log`

[aedwardg]

Given that the main focus of this exercise is to use erlang libaries, it feels strange to receive analyzer errors for the following two solutions:

random_ship_registry/0 (prompts to use Enum.random/1):

def random_ship_registry_number() do
  digits = :rand.uniform(9000) + 999

  "NCC-#{digits}"
end

format_stardate/1 (prompts to use :io_lib.format("~.1f", [stardate]) |> to_string()):

def format_stardate(stardate) do
  stardate
  |> :erlang.float_to_binary([{:decimals, 1}])
end

Would it make sense to have the analyzer allow one or both of these alternate solutions, since they are still using erlang?

[jiegillet]

Hi @aedwardg, Sorry for the slow reply and thank you for opening this issue.

The exercise is teaching two concepts, randomness and using erlang libraries, this explains why we are nudging the students towards Enum.random. However, upon closer look, we are delivering the message

Use :rand.uniform instead of Enum.random in the random_stardate function. :rand.uniform allows you to generate a random float directly, as opposed to Enum.random that only allows you to generate random integers.

which is not fully accurate, according to the docs :rand.uniform(n) does indeed return an integer between 1 and n. In the spirit of learning about Elixir Enum.random function, I think I would argue that using Enum.random(0..n) is more explicit about the range, so it is preferable. What would you think about changing the message to

Use :rand.uniform instead of Enum.random in the random_stardate function. :rand.uniform(n) allows you to generate a random integer 1 <= x <= n, but Enum.random/1 takes a range argument a..b//c, which is more explicit and permissive.

considering the second message, I think this is a good suggestion. :io_lib.format is more powerful, but :erlang.float_to_binary is enough to satisfy all the requirements and should therefore be allowed. As a matter of fact, we should probably accept any solution that uses erlang, not just these two.

Would you like to help with implementing those changes?

[aedwardg]

@jiegillet, I think the following (current) messages is correct in how it's being used for the random_stardate function, since the instructions for that one ask for a float, which is what :random.uniform/0 returns:

Use :rand.uniform instead of Enum.random in the random_stardate function. :rand.uniform allows you to generate a random float directly, as opposed to Enum.random that only allows you to generate random integers

So for that one, I don't think any changes need to be made.

However, for the random_ship_registry_number, I do think it makes sense to include your clarification about Enum.random/1 being more explicit than :random.uniform/1, since that one should return an integer and can be solved with either approach.

I also agree that for the final format_stardate function, it makes sense that any erlang solution should pass the analyzer.

I'd be happy to help out with these changes, though I'll need to familiarize myself with how the analyzer works first 🙂

[jiegillet]

Great, let's do it!

The comments live over here, so a PR over there would be great.

For detecting erlang in the analyzer, what would be required is changing this file, replacing assert_call "format_stardate uses :io_lib" by a check_source that would look into the AST and detect an erlang call inside format_stardate. And add a bunch of tests.
I think it's a pretty complex task, ASTs are tricky, so I would not especially ask of you to do it, unless you feel confident about it.

[lpetic]

I think that this issue can be closed now.

[jiegillet]

Why? The points discussed haven't been addressed yet.