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.
