exercism / elixir-analyzer

GNU Affero General Public License v3.0
30 stars 32 forks source link

DNA Encoding warning for Enum.reverse() #370

Closed chaychoong closed 1 year ago

chaychoong commented 1 year ago

A solution for the decode method:

  def decode(bs), do: Enum.reverse(decode(bs, []))
  defp decode("", result), do: result

  defp decode(<<value::4, rest::bitstring>>, result) do
    decode(rest, [decode_nucleotide(value) | result])
  end

Here, a Enum.reverse is necessary, since the new values are accumulated in the reverse order. However, the following warning is shown:

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 warning should be removed.

Going one step further, the alternative - doing result ++ [decode_nucleotide(value)] - seems to be a very popular community solution, but should be discouraged since the complexity of appending to the end of the list is O(n), n being the length of the list.

angelikatyborska commented 1 year ago

Hi! Thanks for the feedback.

Using Enum.reverse is never strictly necessary. You can always implement your own recursive function that reverses a list, which is exactly what the analyzer comment is trying to get you to do. For reference, this is the ideal solution to the exercise.

the alternative - doing result ++ [decode_nucleotide(value)] - seems to be a very popular community solution, but should be discouraged since the complexity of appending to the end of the list is O(n), n being the length of the list.

Yes, definitely, that should be discouraged. Could you give me 3-4 links to community solutions that use ++ that you found? I looked at the top 12 community solutions, and didn't find any, but to my shock found way too many that don't even use tail call recursion at all :( another potential challenge for the analyzer (I opened a separate issue for that https://github.com/exercism/elixir-analyzer/issues/371).

chaychoong commented 1 year ago

Oh! It didn't occur to me that implementing a reverse function is in the scope of the exercise... that's a good point.

Here are some recent examples: 1, 2, 3, 4, 5

chaychoong commented 1 year ago

Closing this issue