# How to improve this code?

## Background

I have come up with a solution to the Caeser Cipher challenge in Exercism.io. This solution works and passes all tests, but I am still not happy.

## Code

``````defmodule RotationalCipher do
@doc """
Given a plaintext and amount to shift by, return a rotated string.

Example:
iex> RotationalCipher.rotate("Attack at dawn", 13)
"Nggnpx ng qnja"
"""
@spec rotate(text :: String.t(), shift :: integer) :: String.t()
def rotate(text, shift) do
text
|> String.to_charlist()
|> Enum.map( fn char ->
cond do
is_lower_case?( char ) -> rotate_lower_case( char, shift )
is_upper_case?( char ) -> rotate_upper_case( char, shift )
true -> char
end
end )
|> to_string()
end

defp is_lower_case?( char ), do: char >= 97 && char <= 122
defp rotate_lower_case( char, shift ), do: 97 + rem( char - 71 + shift, 26 )

defp is_upper_case?( char ), do: char >= 65 && char <= 90
defp rotate_upper_case( char, shift ), do: 65 + rem( char - 39 + shift, 26 )
end
``````

So, given a string and a shift, I have to shift all the letters. Punctuation, numbers and spaces remain the same.

I achieve this by converting the string to a `charlist`, mapping over it, and then converting the result into a string again. But in my eyes, there is room for better.

## Possible improvements

1. I had to define `is_upper_case?` and `is_lower_case?` for characters. Isnâ€™t there a built in function that already does this?
2. In my `Enum.map` I have a function that I would like to extract into a `defp`, for the sake of clarity. But I still donâ€™t know how to do it and I only found answer that tell me how to use anonymous functions with the terse syntax. How would I call an already defined function that takes more than 1 parameter inside an `Enum.map` ?
3. I am using a conditional, even though I have the feeling I shouldnâ€™t be using one yet. The exercise is supposed to be about string only but I am already using control flow. I have the feeling I am over complicating. Perhaps there is a simpler solution?

Let me know what you guys think!

No, there is not.

``````Enum.map(list, &your_defp_fun(&1, shift))
``````

At least when I understand you correctly.

Donâ€™t take the abscense of the control flow label too strict. those labels are relatively new and they havenâ€™t been applied thoroughly yet. This is a problem common to most tracks.

Have you considered using a `Range` and charliterals? eg. `char in ?a..?z`.

But in general I think it looks quite good already.

1 Like

@Fl4m3Ph03n1x, I think this is a good case for binary pattern matching.

``````defmodule Caesar do
defguard is_lower?(char) when char in ?a..?z
defguard is_upper?(char) when char in ?A..?Z

@spec rotate(text :: String.t() | integer, shift :: integer) :: String.t()
def rotate(<< char :: integer-size(8), rest :: binary >>, shift)  do
<< rotate(char, shift) :: integer-size(8), rotate(rest, shift) :: binary >>
end

def rotate("", _shift), do: ""

def rotate(char, shift) when is_lower?(char), do: ?a + rem( char - ?G + shift, 26 )
def rotate(char, shift) when is_upper?(char), do: ?A + rem( char - ?' + shift, 26 )
def rotate(char, _shift), do: char
end
``````
8 Likes

Thanks for the feedback everyone!

@NobbZ yah, thanks for pointing out about the labels. It was making me feel really worthless not being able to do an exercise that was supposed to be super simple and have no conditionals. Turns all the solutions use some advanced feature, so itâ€™s good to know that I should have a bigger level in Elixir before attempting Exercism. Now I understand why @peerreynders suggested me to read the book first and then do the exercises !

@kip Your solution is probably awesome, but I am too young to appreciate it. This happens because I am still learning the basics of Elixir and I have no idea what guards are, let along all the <<>> thing going on. Along with that binary strings are still a somewhat vague concept. I mean, for one of the 1st exercises in Exercism I would expect something simpler.
I will likely return to this later and try to appreciate it for itâ€™s full glory

Turns all the solutions use some advanced featureâ€¦

Kipâ€™s is amazing with the binary pattern matching, which goes into the string itself like it has superpowers. Hereâ€™s my more pedestrian beginnerâ€™s attempt, with run-of-the-mill guards and very basic pattern matching â€“ no advanced features. Everything here is in the first parts of the Getting Started guide on elixir-lang.org. It does seem easy to read.

``````defmodule RotationalCipher do
def rotate(text, shift) do
text
|> to_charlist()
|> Enum.map(&handle(&1, shift))
|> to_string()
end

def handle(char, shift) when char in ?a..?z, do: spin(char, shift, ?z)
def handle(char, shift) when char in ?A..?Z, do: spin(char, shift, ?Z)
def handle(char, _shift), do: char

def spin(char, shift, z) when char + shift > z, do: char + shift - 26
def spin(char, shift, _z), do: char + shift
end
``````

Everything youâ€™d think of as conditional logic has been moved to the function headers, using guards in this case. Of course you could write a more traditionally conditional version:

``````defmodule RotationalCipher2 do
def rotate(text, shift) do
text
|> to_charlist()
|> Enum.map(&handle(&1, shift))
|> to_string()
end

def handle(char, shift) do
cond do
char in ?a..?z -> spin(char, shift, ?z)
char in ?A..?Z -> spin(char, shift, ?Z)
true -> char
end
end

def spin(char, shift, z) do
if(char + shift > z, do: char + shift - 26, else: char + shift)
end
end
``````

I find the first version more direct.

By the way, AstonJ in this forum has been recommending Dave Thomasâ€™ Elixir for Programmers video course. I tried it and have found it an excellent addition to books because you (or at least I) get a sense of workflow and state of mind along with a great introduction to OTP.

2 Likes

Completely agree with the comment about Dave Thomasâ€™s video course. I also think that Lance Halvorsenâ€™s book, should be on the reading list of anyone wanting to deploy a basic understanding of Elixir to the construction of larger applications.

Getting back to to the topic, this version was inspired by the treatment of comprehensions on bitstrings in the Dave Thomas book (p108 in my 2016 1.3 edition):

``````defmodule Caesar do

@spec rotate(text :: String.t(), shift :: integer) :: String.t()

def rotate(text, shift), do: List.to_string(for <<ch <- text>>, do: caesar_code(ch, shift))

defp caesar_code(ch, shift) when ch in ?a..?z, do: rotate_code(ch, shift, ?z)

defp caesar_code(ch, shift) when ch in ?A..?Z, do: rotate_code(ch, shift, ?Z)

defp caesar_code(ch, _shift), do: ch

defp rotate_code(ch, shift, z), do: if ch + shift > z, do: ch + shift - 26, else: ch + shift

end
``````

Itâ€™s not as elegant as kipâ€™s solution, but it is easy to follow.

1 Like