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.