patternMinor
Haskell encryption tips
Viewed 0 times
encryptionhaskelltips
Problem
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:
Instead of taking a single value as the offset, it takes a
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 Key -> (Int -> Int -> Int) -> Message
alter msg key dir = [(shift c k dir) | (c,k) 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 (-)
-}Solution
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 = 32It'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 -> CharAs 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.Code Snippets
type Direction = (Int -> Int -> Int)upperBound = 127 ; lowerBound = 32shift :: Char -> Char -> (Int -> Int -> Int) -> Char
shift char charoff dir = fix $ dir (ord char) (ord charoff)fix :: Int -> Char| charcode > upperBound = chr $ (lowerBound - 1) + excessOf (upperBound + lowerBound)
| charcode < lowerBound = chr $ (upperBound + 1) - excessOf (lowerBound - lowerBound)
where excessOf boundry = abs(boundry - charcode)Context
StackExchange Code Review Q#54851, answer score: 4
Revisions (0)
No revisions yet.