[Haskell-beginners] Code Review of Caesar-Cipher
chrysaetos99
chrysaetos99 at posteo.de
Thu May 21 06:36:44 UTC 2020
Hi,
thank you for your feedback Daniel an Toz. I now used (most of) your
suggestions to improve the code.
You can find the result as an attachment.
Can I improve this even further? Also, I have put a question in the code
(as a comment). Could anyone answer
that one also?
I would appreciate any answers, and thanks again for your effort.
chrysaetos99
On 21.05.20 02:04, 鲍凯文 wrote:
> Hi,
>
> For what it's worth, Daniel's suggestion of:
>
> ```haskell
> isAlphabetic char = elem char ['A'..'Z']
> ```
>
> does read nicer but would have to traverse `['A'..'Z']` every time. I
> think what you have is fine, and although relational and boolean
> operators are also in imperative languages, they behave as pure
> functions even in imperative languages if subexpressions don't cause
> side effects. I don't think it's unidiomatic, and especially in this
> case, "between A and Z" means the same thing as "is one of the letters
> A, B, C...Z", so the intent of the function is clear as written.
>
> Best of all would probably be using `isAlpha` from `Data.Char` (in
> `base`).
>
> Best,
>
> toz
>
> On Wed, May 20, 2020 at 2:41 PM <beginners-request at haskell.org
> <mailto:beginners-request at haskell.org>> wrote:
>
> Send Beginners mailing list submissions to
> beginners at haskell.org <mailto:beginners at haskell.org>
>
> To subscribe or unsubscribe via the World Wide Web, visit
> http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners
> or, via email, send a message with subject or body 'help' to
> beginners-request at haskell.org <mailto:beginners-request at haskell.org>
>
> You can reach the person managing the list at
> beginners-owner at haskell.org <mailto:beginners-owner at haskell.org>
>
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of Beginners digest..."
>
>
> Today's Topics:
>
> 1. Code Review of Caesar-Cipher (chrysaetos99)
> 2. Re: Code Review of Caesar-Cipher (Daniel van de Ghinste)
>
>
> ----------------------------------------------------------------------
>
> Message: 1
> Date: Wed, 20 May 2020 20:26:37 +0200
> From: chrysaetos99 <chrysaetos99 at posteo.de
> <mailto:chrysaetos99 at posteo.de>>
> To: beginners at haskell.org <mailto:beginners at haskell.org>
> Subject: [Haskell-beginners] Code Review of Caesar-Cipher
> Message-ID: <cd08aa1e-1c9b-a609-15df-5bf37508cb96 at posteo.de
> <mailto:cd08aa1e-1c9b-a609-15df-5bf37508cb96 at posteo.de>>
> Content-Type: text/plain; charset="utf-8"; Format="flowed"
>
> 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
> ----
> See attachment.
>
>
> 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.
>
> ---
>
> Please note: I also asked this question on
> https://codereview.stackexchange.com/questions/242529/caesar-cipher-implementation,
>
> but didn't receive an answer that really answered all my questions.
>
>
> Kind regards
>
> chrysaetos99
>
>
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: caesar.hs
> Type: text/x-haskell
> Size: 1294 bytes
> Desc: not available
> URL:
> <http://mail.haskell.org/pipermail/beginners/attachments/20200520/71a523ea/attachment-0001.hs>
>
> ------------------------------
>
> Message: 2
> Date: Wed, 20 May 2020 23:37:18 +0200
> From: Daniel van de Ghinste <danielvandeghinste at gmail.com
> <mailto:danielvandeghinste at gmail.com>>
> To: The Haskell-Beginners Mailing List - Discussion of primarily
> beginner-level topics related to Haskell
> <beginners at haskell.org <mailto:beginners at haskell.org>>
> Subject: Re: [Haskell-beginners] Code Review of Caesar-Cipher
> Message-ID: <E33F7055-8525-4E09-97E2-3947848F4012 at gmail.com
> <mailto:E33F7055-8525-4E09-97E2-3947848F4012 at gmail.com>>
> Content-Type: text/plain; charset="utf-8"
>
> I’m no guru, so somebody please feel free to correct my advice
> here. I’m trying to get a bit more active in reading these mailing
> lists and giving advice as I believe it’ll help me become better
> in my own understnading.
>
> First thing, well done! It works! Nice job for the beginning of
> your haskell journey :) Ill be focusing on your first and third
> question. Your indentations seem fine here so far.
>
> There are many different approaches you could take as you learn
> more, but here are some things I noticed:
>
> ******************** 1)
> isAlphabetic :: Char -> Bool
> isAlphabetic char = char >= 'A' && char <= ‘Z'
>
> I feel this would be more readable as a literal, though you
> function works. The <= && => feel very much like imperative style
> to me
>
> isAlphabetic :: Char -> Bool
> isAlphabetic char = elem char ['A'..'Z']
>
> That feel nicer to me. ‘Elem' works like ‘in’ in some other
> languages and has the type:
> elem :: (Eq a, Foldable t) => a -> t a -> Bool
>
> Lists are a ‘foldable t’ as would be a binary tree (or some other
> type of tree). Just think of ‘foldable’ as something you can
> iterate over/through. It as something you can iterate over.
>
> The ‘..’ Syntax in the expression [‘A’..’Z’] works on any type
> which is a member of the ‘Enum’ type class, which is to say it has
> some type of successor/predecessor order. A comes after b, c comes
> after b and so on.
> So typing [‘A’..’Z’] just creates a literal list of the
> characters from ‘A’ to ‘Z’.
>
> Come to think of it, the above would be even nicer if you wrote it
> in ‘point free style’ (notice the lack of a ‘char’ in my expected
> isAlphabetic function parameters, despite it being in the type
> signature) which would require you to ‘flip’ the elem function so
> the Char becomes the second argument expected rather than the first
>
> alphabetLength :: Int
> alphabetLength = length alphabet
>
> alphabet :: [Char]
> alphabet = ['A'..'Z']
>
> isAlphabetic :: Char -> Bool
> isAlphabetic = flip elem alphabet
>
> ****************************** 2)
> Try to avoid so much logic in your encrypt and decrypt functions
> (nested ‘if’ ’then’ ‘else’ makes it harder to follow). Also, I
> don’t know why you are having your functions here return an Int
> and letting your ‘encrypt’ and ‘decrypt’ functions do the work of
> changing them back into a Char. It seems to me if you give a Char
> to your encryptChar function, you should get back an encrypted
> Char. Make these functions below finish their work. I assume you
> were tripping yourself up with what to return if isAlphabetic is
> false, but just return the char value unchanged. Then the type
> signature of these 2 functions below could be Char -> Int -> Char
> instead
>
>
> encryptChar :: Char -> Int -> Int
> encryptChar char shift = if isAlphabetic char --
> Only encrypt A...Z
> then (if ord char + shift > ord 'Z'
> -- "Z" with shift 3 becomes "C" and not "]"
> then ord char + shift -
> alphabetLength
> else ord char + shift)
> else ord char
>
> decryptChar :: Char -> Int -> Int
> decryptChar char shift = if isAlphabetic char
> then (if ord char - shift < ord 'A'
> then ord char - shift +
> alphabetLength
> else ord char - shift)
> else ord char
>
> Also, notice how similar the 2 functions are. These could be
> combined. The only difference between them is the ‘+’ and ‘-‘
> operations switching. Maybe something like the following
> shiftChar :: Int -> Char -> Char
> shiftChar n c
> | isAlphabetic c = chr ((((ord c + n) - base) `mod`
> alphabetLength) + base)
> | otherwise = c
> where base = 65
> This version works for negative or positive values of n. So it can
> be used to encrypt and decrypt. My brackets are a bit ugly, but
> I’m getting lazy (it’s late here).
> The use of `mod` here (the infix version of the modulo operator, %
> in some other languages like python) means we can accept
> arbitrarily large values of n to our function, and it just wraps
> around.
> We can also accept negative values.
> The pipe syntax ‘|’ is just a neater way of doing if else
> statements (see guards for more info)
> The 65 here is because ‘ord' is defined for all possible Char
> values. You’re defining your valid range for this cipher to
> capital alphabetic characters, which starts at 65:
>
> *Main> fmap ord ['A'..'Z']
> [65,66,67,68,69,70,71,72,73,74,75,76,77,78,79,80,81,82,83,84,85,86,87,88,89,90]
> — the corresponding number for each capital letter in order.
>
> If you haven’t come across fmap yet, it’s the same as map for
> lists, but you can use it on structures other than lists - see
> Functors later in your Haskell education :)
> basically it applies the function to each of the value in the list
> here
>
> ************************************ 3)
> Lastly, your encrypt and decrypt functions
> 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]
>
> I like your use of list comprehensions here :) though as mentioned
> before I don’t think you should be using the chr function here,
> rather your encryptChar and decryptChar functions should be doing
> that to the values they return.
>
> Because I changed other pieces in the code, I’ll show you another
> way of doing this part now.
>
> encrypt' :: Int -> String -> String
> encrypt' x = fmap (shiftChar x)
>
> decrypt' :: Int -> String -> String
> decrypt' x = encrypt' (negate x)
>
> Here I’m using ‘point free’ style again only referencing the first
> of the two parameters expected by encrypt’. Because we changed our
> other function in answer ******2 on this mail, encrypt’ and
> decrypt’ are just the inverse of each other. So decrypt' is
> defined in terms of encrypt’ but just negates the number passed to
> encrypt’. As you might expect, negate just makes positive numbers
> negative, and negative ones positive.
>
> In encrypt’ I also use fmap again, because in Haskell a String is
> just a type alias for a list of Characters ( type String =
> [Char]), so we are just mapping the function shiftChar, with the
> number already baked in, to each character in the list of
> character (string) which will be passed to this function.
>
> Please find attached my edits to your source file if you wanna see
> it in full and play around with it.
>
> Best regards,
> Lord_Luvat
>
> P.S. Let me know if I’m a bad teacher, or pitching this response
> at the wrong level :) I’m trying to learn here too.
>
>
>
> > On 20 May 2020, at 20:26, chrysaetos99 <chrysaetos99 at posteo.de
> <mailto:chrysaetos99 at posteo.de>> wrote:
> >
> > 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
> > ----
> > See attachment.
> >
> >
> > 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.
> >
> > ---
> >
> > Please note: I also asked this question on
> https://codereview.stackexchange.com/questions/242529/caesar-cipher-implementation,
> but didn't receive an answer that really answered all my questions.
> >
> >
> > Kind regards
> >
> > chrysaetos99
> >
> >
> > <caesar.hs>_______________________________________________
> > Beginners mailing list
> > Beginners at haskell.org <mailto:Beginners at haskell.org>
> > http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners
>
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL:
> <http://mail.haskell.org/pipermail/beginners/attachments/20200520/2bb8f793/attachment.html>
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: Ceasar.hs
> Type: application/octet-stream
> Size: 650 bytes
> Desc: not available
> URL:
> <http://mail.haskell.org/pipermail/beginners/attachments/20200520/2bb8f793/attachment.obj>
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL:
> <http://mail.haskell.org/pipermail/beginners/attachments/20200520/2bb8f793/attachment-0001.html>
>
> ------------------------------
>
> Subject: Digest Footer
>
> _______________________________________________
> Beginners mailing list
> Beginners at haskell.org <mailto:Beginners at haskell.org>
> http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners
>
>
> ------------------------------
>
> End of Beginners Digest, Vol 143, Issue 5
> *****************************************
>
>
> _______________________________________________
> Beginners mailing list
> Beginners at haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.haskell.org/pipermail/beginners/attachments/20200521/095cb9c1/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: caesar.hs
Type: text/x-haskell
Size: 811 bytes
Desc: not available
URL: <http://mail.haskell.org/pipermail/beginners/attachments/20200521/095cb9c1/attachment-0001.hs>
More information about the Beginners
mailing list