# Implement Enum.Split exercise Code Review

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``````

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

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

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

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.

Personally I prefer this:

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

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

but is up to you

2 Likes

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

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

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]}``````

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

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

True, haha thanks for all the help and feedback.

``````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

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)