Accumulate's recursive function does not actually test recursive mapping about elixir HOT 14 CLOSED

Uh, I don't remember ever submitting an exercism. 😄

from elixir.

Is that not you at the bottom of the readme?? http://exercism.io/exercises/elixir/accumulate/readme

from elixir.

That's me. Wild. I wonder what I said???

from elixir.

I believe you misread the actual code from the test. each char is being mapped with the function given, which returns a list with the char and [1,2,3] as strings. the accumulate function does nothing special if the list passed in has a list as an element

I do agree that this test is confusingly named though

from elixir.

So I understood the test name to mean that the map function is applied recursively if a list element is encountered while iterating.

Thats why I put a recursive call in my implementation:
http://exercism.io/submissions/b907c6a12d6e4b0fae4bb664b3b8adbf#L22

Which I found out that the test suite passes without that line since the passed in anonymous function performs the second level "map".

I'm not sure what its trying to test if it doesn't require recursive mapping.

from elixir.

so yes, I think the test name is confusing and should be changed here

from elixir.

Yes, this looks confusing. Would someone mind submitting a PR with a better test description?

from elixir.

Is there a way you can reach this last test and have it fail? I don't see what it drives out, and the test itself is adding non-essential complexity (passing in a function that closes over a list), that has nothing to do with the function tested.

In a code base of my own, I'd remove it. If we change the name, I'd suggest "accumulate lists". Still pretty ambiguous, but perhaps less likely to lead someone to implement a deep map.

from elixir.

@martinsvalin I was suggesting, as you are, that we change the test (leaving the name) to have the user implement a deeply recursive map function. However useless it might be in real life, the exercise would be a good addition.

from elixir.

That's me. Wild. I wonder what I said???

@JEG2 I think this was during the Rogues retreat, and we were casually chatting about mentoring people, and you mentioned that you had gotten some of your mentees to reimplement enumerable methods.

from elixir.

Ah, could be!

from elixir.

I would suggest using accumulate itself instead of for and call it "nested accumulate":

  test "nested accumulate" do
    chars = ~w(a b c)
    nums  = ~w(1 2 3)
    fun = fn(c) -> Accumulate.accumulate(nums, &(c <> &1)) end
    expected = [["a1", "a2", "a3"], ["b1", "b2", "b3"], ["c1", "c2", "c3"]]
    assert Accumulate.accumulate(chars, fun) == expected
  end

from elixir.

I'm scratching my head on this. It looks to me like the original thought was to drive out deep mapping with something like:

test "accumulate recursively" do
    values = [[1,2,3], [4, 5, 6]]
    fun = fn(num) -> num * 2 end
    expected = [[2, 4, 6], [8, 10, 12]]
    assert Accumulate.accumulate(values, fun) == expected
  end

@tejasbubane the change in your PR illustrates that Accumulate.accumulate/2 can take a function that allows it to compute the permutations of two lists. However, I don't see it driving out any additional behavior. This test, as @martinsvalin points out, has the non-essential complexity of closing over num in the passed function.

My recommendation is that we either take this test out or make it drive out additional behavior.

from elixir.

@parkerl Agreed. It does not drive any additional behaviour. My idea was to give a better description to the test (nested instead of recursive) and since we already have accumulate use that itself instead of for. I am perfectly fine with removing the test entirely since it does not test anything new which the rest of the tests don't.

from elixir.