DNA Encoding should only complain about using builtin iteration constructs only in function bodies

[christhekeele]

On my first iteration of this exercise, I was warned:

The purpose of this exercise is to teach recursion. Solve it without using list comprehensions or any of the functions from the modules Enum, List, or Stream.

This is a fair point--I'd missed the context of the lesson and used for in my solution bodies.

I rewrote it to create my second iteration, and got the same warning. This is because I'm still using for in the module body just to define my mapping functions, which I think is valid and does not "skip" the point of the lesson.

I'd argue we should only be checking function bodies for usage of these constructs, not the entire AST of the submitted module.

[angelikatyborska]

Hi!

The link you posted is not a permanent link to your solutions, but a generic link under each student can see their own solutions. So I'm writing this response without actually seeing your solution 🙂 if you think seeing it would change my mind, please copy-paste it!

Still I am very confident that you should not be using for anywhere in your solution. You should really be only using tail call recursion to iterate over lists and bitstrings in this exercise. If you're using for for anything, you either have unnecessary code in your solution, or you haven't used tail call recursion where the exercise expected you to.

The ideal solution contains three usages of tail call recursion - once in encode/1, and twice in decode/1, because decode/1 also needs a reverse function at the end. This is an ideal solution:

defmodule DNA do
  def encode_nucleotide(?\s), do: 0b0000
  def encode_nucleotide(?A), do: 0b0001
  def encode_nucleotide(?C), do: 0b0010
  def encode_nucleotide(?G), do: 0b0100
  def encode_nucleotide(?T), do: 0b1000

  def decode_nucleotide(0b0000), do: ?\s
  def decode_nucleotide(0b0001), do: ?A
  def decode_nucleotide(0b0010), do: ?C
  def decode_nucleotide(0b0100), do: ?G
  def decode_nucleotide(0b1000), do: ?T

  def encode(dna) do
    do_encode(dna, <<>>)
  end

  defp do_encode([], acc), do: acc

  defp do_encode([n | rest], acc) do
    do_encode(rest, <<acc::bitstring, encode_nucleotide(n)::4>>)
  end

  def decode(dna) do
    do_decode(dna, [])
  end

  defp do_decode(<<>>, acc), do: acc |> reverse()

  defp do_decode(<<n::4, rest::bitstring>>, acc),
    do: do_decode(rest, [decode_nucleotide(n) | acc])

  defp reverse(l), do: do_reverse(l, [])
  defp do_reverse([], acc), do: acc
  defp do_reverse([h | t], acc), do: do_reverse(t, [h | acc])
end

The learning exercises were designed in a way that every single tiny function matters. That's why we're checking the whole AST. There are concepts in Elixir which can be exchangeable, and it's perfectly fine to use whichever concept you prefer in your day to day Elixir code. However, in those learning exercises, we do strict checks to ensure that people get the most value out of them and don't get stuck in practice exercises later. For this reason, the exercise to learn recursion and tail call recursion doesn't allow any other method of iteration, the exercise to learn list comprehensions doesn't allow using the Enum module or the List module and so on.

[christhekeele]

Oh no, of course it's personalized—didn't even look at the link before markdowning it! Let me just share a snippet here, to better illuminate my point:

This is because I'm still using for in the module body just to define my mapping functions, which I think is valid and does not "skip" the point of the lesson.

You mention:

Still I am very confident that you should not be using for anywhere in your solution. You should really be only using tail call recursion to iterate over lists and bitstrings in this exercise. If you're using for for anything, you either have unnecessary code in your solution, or you haven't used tail call recursion where the exercise expected you to.

I raise the issue because I was using tail call recursion exclusively where expected (inside encode/decode functions), but perhaps for where not expected (to define encode_nucleotide/decode_nucleotide):

defmodule DNA do

  @mapping %{
    ?\s => 0b0000,
    ?A => 0b0001,
    ?C => 0b0010,
    ?G => 0b0100,
    ?T => 0b1000
  }
  for {char, num} <- @mapping do
    def encode_nucleotide(unquote(char)), do: unquote(num)
  end
  for {char, num} <- @mapping do
    def decode_nucleotide(unquote(num)), do: unquote(char)
  end

  #...rest of the owl
end

I would argue this is neither unnecessary code, nor not using tail call recursion where the exercise expected to.

As the exercise only expects you to use tail recursion inside function bodies (the only place you can), my proposed suggestion was to have this constraint only apply within defs.

Scanning through the constraints in question and the assert_no_call macro implementation, though, I can see that this is not really supported by the constraint DSL today, and this is a nitpicky enough of a gripe that I can see it not being worth it if it's never felt valuable in other exercises' constraints!

[christhekeele]

We could include some sort of notion of "context" in the Macro.prewalk accumulator, add clauses in the callback fn that recognize AST nodes corresponding to descent into such contexts as "in module definition", "in function definition", "in type definition", etc... And then extend the DSL to specify in what contexts calls are[n't] allowed, avoiding emitting warnings as appropriate.

But this seems like overkill for what is not a common piece of metaprogramming (for ... do: def ...) in an early concept. If this capability doesn't sound useful for other exercises, I'd definitely close this!

[angelikatyborska]

I would argue this is neither unnecessary code, nor not using tail call recursion where the exercise expected to.

Absolutely! Now that I see your code, I totally agree with you and I feel stupid for my previous comment 😅 . It didn't even cross my mind that somebody was using list comprehensions for metaprogramming on Exercism.

I have to admit we developed the analyzer with the assumption that metaprogramming can be ignored because it's more important that the analyzer gives valuable feedback to newcomers rather than experienced Elixir devs who can figure out for themselves if they're doing something right or wrong.

We could include some sort of notion of "context" in the Macro.prewalk accumulator, add clauses in the callback fn that recognize AST nodes corresponding to descent into such contexts as "in module definition", "in function definition", "in type definition", etc

Having a "in module definition" vs "in function/macro definition" distinction for assert_call could definitely be useful for the future. But right now, I am not sure if we ever need it for "in module definition". There are no concepts about metaprogramming and I'm not sure if there ever will be. I mean, I would love to have them, but I don't know if I will ever have the energy and creativity necessary to create them myself!

Maybe for now it would be enough to limit assert_call and asert_no_call to completely ignore calls that happen outside of function/macro bodies? @jiegillet What do you think?

[jiegillet]

I agree that we could limit assert_call and asert_no_call to in function/macro. We already keep track of the function definition context via in_function_def so maybe we can use that?