5
\$\begingroup\$

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
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.