Implement Enum.Split exercise Code Review

Questions / Help
1

Hi all, I’m currently learning Elixir and reading “Programming Elixir 1.3”.

I just completed one of the exercises of the book about implementing some Enum functions without library functions and comprehensions in this scenario is the “split” functions. But I wanted some input and feedback about what I ended doing. Any help or feedback is greatly appreciated.

Thanks

defmodule Functions do
    def split(list, count), do: split(list, count, {[],[]})
    defp split(list, count, res) when count == 0 or length(list) == 0 do
        res
    end
    defp split(list, count, res) when count > 0 do
        [h|t] = list
        res = {elem(res, 0) ++ [h], t}
        split(t, count - 1, res)
    end
    defp split(list, count, res) when count < 0 do
        cond do
            length(list) + count > 0 -> 
                split(list, length(list) + count, res)
            length(list) + count == 0 ->
                res = {[], list}
                split(list, 0, res)
    end
end
2

I haven’t read the book, what’s the task here? To split a list in two?

Preliminary remark: use list == [] instead of length(list) == 0 in guards. (1) because it’s faster – length will traverse the entire list if it is not empty, while comparing with the empty list is instant – and (2) because from Elixir 1.7 and on you will get a compiler warning if you use length(list) == 0 in guards.

1 Like
3

It’s basically to implement the same functionality of Enum.split/2 to practice Elixir.

4

Your code reads on a first glance as if split(list, count) shall return a tuple {xs, ys} where length(xs) <= count and length(ys) == length(list) - length(xs). Is this correct? Or backwards on negative count.

If yes, then let me walk through your code, ignore me otherwise and give us an excerpt of the exercise, as not everyone might have that book, but code reviews are easier if the goal of the task is known.

  1. As @dimitarvp already said, you should prefer list == [] or a match over a length check, as every call to length will iterate the full list.
  2. In my opinion you should split the cases count == 0 and list == [] in separate heads, such that you can directly match on 0 or [], this enables the erlang layer to do additional optimisations
  3. use more function head pattern matching.
  4. keep xs and ys distinct and only create the tuple when you return. Constant recosntruction of a new one has a slightly (but measurable) negative impact on memory consumption and runtime

Thats said (and without beeing able to test right now, especially in the reverse part there might be some brainflaw) I’d do it like this:

def split(list, count) when is_integer(count) && count < 0, do: split(:neg, -count, [], reverse(list))
def split(list, count), do: split(:pos, count, [], list)

defp split(:neg, 0, xs, ys), do: {reverse(xs), reverse(ys)}
defp split(:pos, 0, xs, ys), do: {xs, ys}
defp split(:neg, _, xs, []), do: {reverse(xs), []}
defp split(:pos, _, xs, []), do: {xs, []}
defp split(dir, cnt, xs, [h|t]), do: split(dir, cnt - 1, [h|xs], t)

PS: This code assumes that you already have implemented a reverse/1 function.

2 Likes
5

Your code reads on a first glance as if split(list, count) shall return a tuple {xs, ys} where length(xs) <= count and length(ys) == length(list) - length(xs). Is this correct? Or backwards on negative count.

Yes it’s exactly that.

Awesome, thanks for all the feedback.

6

Personally I prefer this:

def split([], count, res), do: res
def split(list, 0, res), do: res

instead of

def split(list, count, res) when count == 0 or length(list) == 0 do
    res
end

but is up to you

2 Likes
7

Agree I think it’s better to have separate function heads

8

I was able to test and compare with the examples of Enum.split/2 now and as I said, I got the reverse part wrong and repaired my code accordingly:

defmodule M do
  def reverse(l), do: Enum.reverse(l)

  def split(list, count) when is_integer(count) and count < 0, do: split(:neg, -count, [], reverse(list))
  def split(list, count), do: split(:pos, count, [], list)

  defp split(:neg, 0, xs, ys), do: {reverse(ys), xs}
  defp split(:pos, 0, xs, ys), do: {xs, ys}
  defp split(:neg, _, xs, []), do: {[], xs}
  defp split(:pos, _, xs, []), do: {xs, []}
  defp split(dir, cnt, xs, [h|t]), do: split(dir, cnt - 1, [h|xs], t)
end

IO.inspect M.split([1,2,3], 2)
IO.inspect M.split([1,2,3], 10)
IO.inspect M.split([1,2,3], 0)
IO.inspect M.split([1,2,3], -1)
IO.inspect M.split([1,2,3], -5)

$ elixir foo.exs
{[2, 1], [3]}
{[3, 2, 1], []}
{[], [1, 2, 3]}
{[1, 2], [3]}
{[], [1, 2, 3]}
1 Like
9

Actually i think the reverse is not needed.
See. (I think it’s only in the positive count scenarios)

Enum.split([1,2,3], 2)
{[1, 2], [3]}
10

In fact, the reverse is needed, but at different places than I have… Such errors were much easier to spot if I had a proper testsuite :wink:

But I do not have any time anymore for another refactoring round, as I have to take my son out of the bathtub :wink:

11

True, haha thanks for all the help and feedback.

12 
defmodule Test do
  def take(_, []), do: []
  def take(0, _), do: []
  def take(n, [a | as]), do: [a | take(n-1, as)]
  def drop(_, []), do: []
  def drop(0, a ), do: a
  def drop(n, [_ | as]), do: drop n-1, as
  def split(n, list), do: {take(n, list), drop(n, list)}
end
2 Likes
13

That would be my take on the exercise:

  def split([], _count), do: {[], []}
  def split(list, 0), do: {[], list}
  def split(list, count) when count < 0, do: split(list, length(list) + count)

  def split([head | tail], count) do
    {first, second} = split(tail, count - 1)
    {[head | first], second}
  end