Fl4m3Ph03n1x

Fl4m3Ph03n1x

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!

Marked As Solved

malloryerik

malloryerik

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.

Also Liked

kip

kip

ex_cldr Core Team

@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
NobbZ

NobbZ

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.

datadrover

datadrover

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.

Where Next?

Popular in Questions Top

srinivasu
How to handle excepions in elixir? Suppose i have A, B, C ,D, E modules. and each module has get() function. A.get() method will call th...
New
gshaw
What is the idiomatic way of matching for not nil in Elixir? E.g., First way: defp halt_if_not_signed_in(conn, signed_in_account) when...
New
ycv005
I have followed this StackOverflow post to install the specific version of Erlang. And When I am running mix ecto.setup then getting fol...
New
SoCreat
i’m a new one to elixir which editor can i use vs code? or atom? Thanks! :smiley:
New
Jim
As a follow up to my earlier question: I have the code compiling and running but not getting a successful login from the rest server. ...
New
sacepums
Hey guys. I'm new to elixir and im really stocked about it. But I ran into a bit of problem - I need to convert a date sting, for examp...
New
stefanchrobot
What’s the safe way to decode a JSON string into a struct? I want to avoid calling String.to_atom. Jason.decode can give me a map with st...
New
mgjohns61585
Could someone help me? I'm making my first elixir program, number guessing game. I can't figure out how to convert the user's guess from ...
New
belgoros
I’m not a pro in using Regex and can’t figure out why the following behaviour happens, especially if we take into account the difference ...
New
sabri
Can someone explain the settings of pool_size of Ecto in config file? and what is the recommend size? Thanks
New

Other popular topics Top

Brian
What is the proper way to load a module from a file in to IEX? In the python world, doing something like this pretty standard: from ....
New
yurko
Here are few pieces of (common) Linux knowledge that we use for reasonably small one server apps. We use Ubuntu but this should work for ...
New
shahryarjb
Hello, I get Persian date from my client and convert it to normal calendar like this: def jalali_string_to_miladi_english_number(persi...
New
William
I would like to know that is there any online source for learning Phoenix Framework for building E-Commerce Store? Any advantage on build...
New
itssasanka
Hi all, Trying to get some more clarity over utc_datetime and naive_datetime for Ecto: https://hexdocs.pm/ecto/Ecto.Schema.html#module-...
New
_russellb
I want to try my hand at web scraping. What tools/libraries do I need to use. I’m hoping to turn this into something professional so don’...
New
grych
Hi folks, Few months ago I have announced the proof-of-concept of the library to manipulate the browsers DOM objects directly from Elixi...
639 49522 488
New
malloryerik
Hi, this is for people who, like me, have had some friction using .html.heex templates in VSCode. The solution seems to be, in a hyphena...
New
vertexbuffer
Hello, can anybody help here..? I have a list of players and I what to delete an element, but every for loop the list is reverting to ori...
New
johnnyicon
Hi all, I've just started learning Elixir and Phoenix Framework, so please pardon my n00bness at this stage. I'm trying to use Postg...
New

We're in Beta

About us Mission Statement