Codewars kata : reverse and rotate

Question

I just started to learn Elixir and to train on Codewars. I solved this kata but I'm not very satisfied of my code and I have no clue (literally) how to improve it. Recursion seems the way to go here, but I'm convinced the code can be optimized a little more.

Here is the full problem :

The input is a string str of digits. Cut the string into chunks of size sz (ignore the last chunk if its size is less than sz).

If a chunk represents an integer such as the sum of the cubes of its digits is divisible by 2, reverse it; otherwise rotate it to the left by one position. Put together these modified chunks and return the result as a string.

Could you help me please ?

defmodule Revrot do
def revrot(str, sz) do
 if str == "" or sz <= 0 or String.length(str) < sz do
 ""
 else
 chunk = str |> String.slice(0,sz) 
 t = chunk |> String.to_integer |> Integer.digits |> Enum.reduce(0, fn(x, acc) -> (:math.pow(x,3) |> round) + acc end)
 temp = case (rem t,2) do
 0 -> chunk |> String.reverse
 _ -> String.slice(chunk, 1, sz-1) <> String.first(chunk)
 end
 temp <> revrot(String.slice(str,sz..-1), sz)
 end
 end 
end

Answer 1

So, the only major comment I'd have on your code is that it's a bit hard to understand what's going on. It becomes a lot easier when you have the Kata text to help, but ideally the code itself should be quite easy to understand just by looking at it.

I would split your code up into the seperate stages, rather than having a single function that does everything. It should make it much easier to see what the code is actually doing at a glance. There's also a few built in elixir functions that you can use to simplify things.

So, essentially you have three stages:

• Split the string into chunks.
• Rotate or reverse each chunk.
• Put the string back together.

I would write a function for each of these stages, then you can compose those functions to get the full process.

First, chunking. You could do this manually, but there's actually already a function that does this in Elixir called Enum.chunk. So you can convert the string into a list of it's unicode codepoints, chunk it, then join it back into a string, like so:

def chunks(str, sz) do
 str |> String.to_charlist |> Enum.chunk(sz) |> List.to_string
end

Next, we'll need a function to check if we should reverse or not. You're implementation doesn't seem bad, but:

• Elixir provides Enum.sum, which is easier than implementing the sum manually with Enum.reduce.
• I would split the sum and the pow operations into two stages.
• I wouldn't be afraid to use newlines and whitespace: it can be easier to read than a single line with a lot of operations going on.

So we could write this like:

def cube_digits(chunk) do
 chunk 
 |> String.to_integer 
 |> Integer.digits 
 |> Enum.map(fn (x) -> x |> :math.pow(3) |> round end)
end
def should_reverse?(chunk) do
 (chunk |> cube_digits |> Enum.sum |> rem(2)) == 0
end

Your rotate code seemed fine, but lets pull it into a function:

def rot_left(chunk) do
 String.slice(chunk, 1, -1) <> String.first(chunk)
end

Then you can put this all together into a single function, making use of the into argument to for to join everything back together again:

def revrot(str, sz) do
 for chunk <- chunks(str, sz), into: "" do
 if should_reverse?(chunk) do 
 String.reverse(chunk)
 else 
 rot_left(chunk)
 end
 end
end

If you were performing some more complicated operations inside the for it might have been worth using pipes along with Enum.map & Enum.join instead of for, but for this operation I think for ends up nice and concise.