3
\$\begingroup\$

I wrote a variation of the Caeser Cipher and would like some feedback. Is it idiomatic? Can you generally see anything that can be improved?

Use example:

let e = enc "Hello World" "This is the key"
 in dec e "This is the key"

Instead of taking a single value as the offset, it takes a String as the key and uses each respective character from the key as the offset; wrapping when it reaches the upper/lower bound.

module Encryption where

import Data.Char

type Key = String
type Message = String
type Direction = (Int -> Int -> Int)

--Upper and lower ascii code bounds
upperBound = 127 ; lowerBound = 32

shift :: Char -> Char -> (Int -> Int -> Int) -> Char
shift char charoff dir = fix $ dir (ord char) (ord charoff)

fix :: Int -> Char
fix charcode
        | charcode > upperBound = chr $ (lowerBound - 1) + excessOf (upperBound + lowerBound)
        | charcode < lowerBound = chr $ (upperBound + 1) - excessOf (lowerBound - lowerBound)
        | otherwise = chr charcode
        where   excessOf boundry = abs(boundry - charcode)

alter :: Message -> Key -> (Int -> Int -> Int) -> Message
alter msg key dir = [(shift c k dir) | (c,k) <- zip msg (cycle key)]

enc msg key = alter msg key (+)
dec msg key = alter msg key (-)

--Basic Caeser Cipher
{-
alter :: Message -> Key -> Direction -> Message
alter m k dir = [chr $ dir (ord out) k | out <- m]

enc m k = alter m k (+)
dec m k = alter m k (-)
-}
\$\endgroup\$
4
\$\begingroup\$
type Direction = (Int -> Int -> Int)

These parentheses are superfluous, though they don't really hurt if they help you to read the code.

upperBound = 127 ; lowerBound = 32

It's not very common to put definitions on the same line with ;, particularly top-level definitions, so this made me double-take a bit. It makes it hard to find a particular definition by scanning the left column of the file.

It's a good idea to give all top-level definitions type signatures, as it helps to catch type errors earlier and makes it easier for someone reading the code.

Also, with numeric types, overloading in the Num class can often give surprising results, so explicitly stating that these are Int is a good idea.

shift :: Char -> Char -> (Int -> Int -> Int) -> Char
shift char charoff dir = fix $ dir (ord char) (ord charoff)

Consider putting the function argument first here. The function is more likely to be useful partially applied as shift dir than as shift char, and it also makes for a more intuitive type signature which correctly gives the sense that a function Int -> Int -> Int is being transformed into a function Char -> Char -> Char.

fix :: Int -> Char

As someone mentioned on your StackOverflow post, fix is a standard library name for something different, so best not to reuse it here.

| charcode > upperBound = chr $ (lowerBound - 1) + excessOf (upperBound + lowerBound)
| charcode < lowerBound = chr $ (upperBound + 1) - excessOf (lowerBound - lowerBound)
    where   excessOf boundry = abs(boundry - charcode)

I find it hard to understand these two cases. You could perhaps add comments to explain what they are doing. Also lowerBound - lowerBound is just 0 so what are you trying to do here? I presume the whole thing is about clamping to within the valid ASCII range but it's quite confusing.

In general I'd have expected a Caesar cipher just to work on letters, rather than the whole ASCII range, but that's a specification question really.

alter :: Message -> Key -> (Int -> Int -> Int) -> Message
alter msg key dir = [(shift c k dir) | (c,k) <- zip msg (cycle key)]

Again I would put the function as the first argument here. I would also put the Key as the second argument, because then you can partially apply it with a key to get a specialised encryption or decryption function. It might also make sense to have the key first and the function second.

enc msg key = alter msg key (+)
dec msg key = alter msg key (-)

As before, type signatures would help here. Also, if you do change the order of the arguments to alter above then this can be abbreviated as enc = alter (+) or enc key = alter key (+), which is perhaps slightly more elegant.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you, that's very helpful. The fix function "wraps" characters falling outside of the upperBound/lowerBound range (s character with the code 128 becomes 32). I literally just scratched that part out on some paper. I'll see if I can either rework it so it's more intuitive, or figure out a descriptive comment. Thanks again! (I forgot to delete the SO post :/. I bet I have like 10 down votes by now.) \$\endgroup\$ – Carcigenicate Jun 21 '14 at 12:02
  • \$\begingroup\$ I'd have expected something with mod to do the wrapping - if you subtract lowerBound, use mod n (upperBound - lowerBound) to force it into the right range, then add back lowerBound, that should roughly cover it. \$\endgroup\$ – GS - Apologise to Monica Jun 21 '14 at 12:20
  • \$\begingroup\$ Thanks, I'll look into mod. I've never even heared of that function. \$\endgroup\$ – Carcigenicate Jun 21 '14 at 12:22
  • \$\begingroup\$ Oh, mod is just a synonym for rem? It looks like rem requires a lot more resources though. \$\endgroup\$ – Carcigenicate Jun 21 '14 at 14:28
  • \$\begingroup\$ They're not quite the same: stackoverflow.com/questions/339719/… - I think you need mod here. Apparently rem is more efficient but I doubt that's of any relevance in this code. \$\endgroup\$ – GS - Apologise to Monica Jun 21 '14 at 14:30

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.