4
\$\begingroup\$

Background

I am a total beginner in Haskell, so after reading the Starting out-chapter of "Learn you a Haskell", I wanted to create my first program that actually does something.

I decided to do the famous Caesar-Cipher.

Code

import Data.Char

encryptChar :: Char -> Int -> Int
encryptChar char shift = if ord char > 64 && ord char < 91    --Only encrypt A...Z
                           then (if ord char + shift > 90     --"Z" with shift 3 becomes "C" and not "]" 
                                  then ord char + shift - 26
                                  else ord char + shift)
                         else ord char

decryptChar :: Char -> Int -> Int
decryptChar char shift = if ord char > 64 && ord char < 91
                            then (if ord char - shift < 65
                                   then ord char - shift + 26
                                   else ord char - shift)
                         else ord char

encrypt :: String -> Int -> String
encrypt string shift = [chr (encryptChar (toUpper x) shift) | x <- string]    --"Loop" through string to encrypt char by char

decrypt :: String -> Int -> String
decrypt string shift = [chr (decryptChar (toUpper x) shift) | x <- string]

main = print(decrypt "KHOOR, ZRUOG!" 3)

(The code does work as intended.)

Question(s)

  • How can this code be improved in general?
  • Do I follow the style guide of Haskell (indentation, etc.)?
  • I have a background in imperative languages. Did I do something that is untypical for functional programming languages?

I would appreciate any suggestions.

\$\endgroup\$

2 Answers 2

Reset to default
5
\$\begingroup\$

  1. I'd start by avoiding directly encoding ASCII character codes into the logic. For example, instead of:

    if ord char > 64 && ord char < 91

    I'd probably use:

    if char >= 'A' && char <= 'Z'

    I think this shows the intent enough more clearly to be worthwhile.

  2. Given that you also do this a couple of different places, I'd probably write a small isUpper function to return a Bool indicating whether a character is an upper-case letter:

    isUpper :: Char -> Bool
isUpper char = char >= 'A' && char <= 'Z'

    Then the rest of the code can use that:

    encryptChar char shift = if isUpper char     -- ...

decryptChar char shift = if isUpper char     -- ...

[Note: the standard library already has an isUpper, but it may not fit your needs, since it's Unicode-aware, and here you apparently only want to deal with English letters.]

\$\endgroup\$
4
\$\begingroup\$

This is mostly fine but there is a lot of repetition as well and it is not broken up very well. Remember that haskell is lazy so none of the operations in the where clauses will be executed unless they are needed, so it is safe to just set them up. Keep in mind I went to the extreme and made a where clause for everything but you can keep it more sensible if you want.

decryptChar char shift  =
  if inRange
    then if wouldWrap
      then wrapped
      else iShifted
    else i
  where
    i = ord char
    inRange = i > 64 && i < 91
    iShifted = i - shift
    wouldWrap = iShifted < 65
    wrapped = iShifted + 26
\$\endgroup\$

You must log in to answer this question.