3
\$\begingroup\$

I'm learning Haskell, and I tried to do an exercise which it is an implementation of the Caesar Cipher method.

The problem asks for cipher a text which it is in lower case and can contain characters from 'a' to 'z' without numbers, uppercase letters, symbols etc.., only lower case letters.

If the letter it is in an even position inside the message, I need to change for itself but in upper case. Example 'a' will be 'A'

If the letter is in an odd position inside the message, I need to change it by an other letter "n" positions next in the alphabet by the original one. Example 'a' will be 'd' if "n" is equal to 3

Important thing, the alphabet needs to be circular, so, after z, will be the 'a'

So you call the function with a text and a number "n" for example: alfabet 3 The result will be: AoFdBhT.

This is my personal implementation, I checked it, and works fine, but I think maybe it is not a very good implementation, and I would like some advice to improve it.

caesarCipher :: String -> Int -> String
caesarCipher = caesarCipherAux [] 0

caesarCipherAux :: String -> Int -> String -> Int -> String
caesarCipherAux codedMessage _ [] _ = codedMessage
caesarCipherAux codedMessage pos (xs:s) n = 
if even (pos) then
    caesarCipherAux (codedMessage++[toUpper xs]) (pos+1) s n
else
    caesarCipherAux (codedMessage++ [cipher xs n]) (pos+1) s n

cipher :: Char -> Int -> Char
cipher xs n
| xs == 'x'='a'        
| xs == 'y'='b'
| xs == 'z'='c'
| otherwise = [xs..'z'] !! n
\$\endgroup\$
1
\$\begingroup\$

Heres my implementation, I abused pattern matching so that way I don't need to check for even and odd index. Your cipher function will also only work for 3, using ord and modulo however will cover all wrap around cases for all integers.

All the below line is doing is getting the next n circular element in our alphabet

[chr (((n + ord y - ord 'a') 'mod' 26) + ord 'a')]

cipher :: [Char] -> Int -> [Char]
cipher [] _ = []
cipher (x:[]) _ = [toUpper x] 
cipher (x:y:xs) n = [toUpper x] ++ [chr (((n + ord y - ord 'a') `mod` 26) + ord 'a')] ++ cipher xs n
\$\endgroup\$
  • \$\begingroup\$ I'm not sure this is an actual review of OP's code... may want to edit and improve to focus on OP's code and adding rationale for OP to understand the points you want to make. \$\endgroup\$ – code_dredd Oct 2 '18 at 3:52
2
\$\begingroup\$

I'm also in the (early) process of learning Haskell, so please take my advice with a grain of salt.

First, let's talk about your coding style

  • I am assuming the indentation is only messed up in your question, as the code in your post would not compile.
  • I am also going to assume that you are either supposed to implement the cipher recursively or don't know a better method, yet (map). You also seem not to know about function composition (using . and $). In case you do, you should generally* use it wherever possible (*unless it reduces readability, but I haven't seen such a case yet).
  • xs is not used to represent scalar (single) values, but values such as lists that can contain many x's. Pattern matching a list using cons is often written as x:xs, f:fs, etc.
  • Using guards to check the same value for equality multiple times is redundant. Using a case ... of expression is more concise and readable:

    cipher x n = case x of
      'x' -> 'a' 
      'y' -> 'b'
      'z' -> 'c'
      _   -> [xs..'z'] !! n
    -- alternatively: cipher 'x' n = 'a' etc.
    
  • even (pos) can be written as even pos.

  • I think using guards instead of if .. then .. else would make your code more readable. Instead of

    caesarCipherAux codedMessage pos (xs:s) n = 
      if even (pos) then
        -- things
      else
        -- other things
    

    write

    caesarCipherAux codedMessage pos (x:xs) n
      | even pos  = -- things
      | otherwise = -- other things
    
  • Your use of whitespace is not always consistent (++ in lines 8 & 10).

  • Consider surrounding infix operators by whitespace to increase readability. In my opinion something like f (x+1) is fine, but | xs == 'x'='c' decreases readability.

  • Avoid ++. Appending lists repeatedly can get somewhat costly (in this case the cost is negligible, but when theres a better way why not choose it). Given that you only append a char, a better way of doing so is consing it: toUpper xs : codedMessage instead of codedMessage++[toUpper xs]. This means, of course, that the end result needs to be reversed: caesarCipher = reverse . caesarCipherAux [] 0 with function composition or caesarCipher = reverse (caesarCipherAux [] 0) without. In cases where you want to append two lists, such as two strings, you can use a list of strings, cons (:) the lists and concat them at the end.

  • Avoid !! and if you use it, make sure its safe. caesarCipher "Hello" 30 will result in an exception. Instead of [xs..'z'] !! n use something like

    let abc = [xs..'z']
    in abc !! mod (length abc) n
    

    Unless abc is empty, this is safe. abc will be empty if xs > 'z', so you have to check that beforehand. Still, not using !! in the first place would be better.

About the implementation

I think your implementation is a good effort for a beginner, but flawed. As long as letters are not shifted past z (or a, if n is negative), it works. It they are, your code either a) yields the wrong result or b) crashes.

a) You tried to implement wrapping letters by hardcoding cases for 'x', 'y' and 'z', but those are only correct for n=3. If n is not 3, the result will be incorrect: caesarCipher "xyz" (-1) is "XbZ" in your implementation (y + 3 = b, where y, b are numerical representations of 'y', 'b'). But it should be "XxZ" because y - 1 = x.

b) Applying caesarCipher to "xwz" 10 results in an exception (*** Exception: Prelude.!!: index too large) because ['w'..'z'] only contains four characters and you did not harcode a case for 'w' (or any of the other letters).

Also, your function does not handle characters outside of ['a'..'z']. I assume that this is outside the scope of your exercise.

Fixing it

Prelude contains ord :: Char -> Int and chr :: Int -> Char to convert a Char to Int and vice-versa. In conjunction with mod, which returns the remainder of an Integral division, those functions are enough to implement letter shifting accurately and safely. Consider this example (w/o function composition):

shift :: Char -> Int -> Char
shift ch n = chr x'
    where r  = 26 -- length alphabet
          a  = ord 'a'
          x  = ord ch
          x' = a + mod (x - a + n) r

works      = shift 'a' 26 == 'a'
stillworks = shift 'b' (-26) == 'b'
wontbreak  = shift 'c' (26 * 4234789) == 'c'

This implementation fixes both problems because it does not use unsafe functions such as !! and works on any character (no hardcoded cases). If you apply it to characters outside the 'a'..'z' range, it will still evaluate successfully and the result will be constrained to said range (which is probably not what you want, so only apply this function to characters that are in the alphabet). To shift upper case letters properly, convert them to lower-case first before applying the function, then they can be converted back (again, not using composition):

foo (x:xs) n acc
    | isUpper x = foo xs n ((toUpper (shift (toLower x))):acc)
    | otherwise = foo xs n (shift x:acc)

Finally, instead of checking for an even/odd number, you can take two characters at once; one will be even, the other odd:

foo []       = "null"
foo [x]      = "x is even"
foo (x:y:xs) = "x is even, y is odd"

Thats all, I think. I hope it helps.

\$\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.