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
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
- 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
|> Enum.map(fn (x) -> x |> :math.pow(3) |> round end)
def should_reverse?(chunk) do
(chunk |> cube_digits |> Enum.sum |> rem(2)) == 0
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)
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
If you were performing some more complicated operations inside the for it might have been worth using pipes along with
Enum.join instead of
for, but for this operation I think
for ends up nice and concise.