Code Review of Caesar-Cipher

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Code Review of Caesar-Cipher

chrysaetos99
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



_______________________________________________
Beginners mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners

caesar.hs (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Code Review of Caesar-Cipher

Daniel van de Ghinste (Lord_Luvat)
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 <[hidden email]> 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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners


_______________________________________________
Beginners mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners

Ceasar.hs (677 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Code Review of Caesar-Cipher

鲍凯文
In reply to this post by chrysaetos99
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 <[hidden email]> wrote:
Send Beginners mailing list submissions to
        [hidden email]

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
        [hidden email]

You can reach the person managing the list at
        [hidden email]

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 <[hidden email]>
To: [hidden email]
Subject: [Haskell-beginners] Code Review of Caesar-Cipher
Message-ID: <[hidden email]>
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 <[hidden email]>
To: The Haskell-Beginners Mailing List - Discussion of primarily
        beginner-level topics related to Haskell <[hidden email]>
Subject: Re: [Haskell-beginners] Code Review of Caesar-Cipher
Message-ID: <[hidden email]>
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 <[hidden email]> 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
> [hidden email]
> 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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners


------------------------------

End of Beginners Digest, Vol 143, Issue 5
*****************************************

_______________________________________________
Beginners mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners
Reply | Threaded
Open this post in threaded view
|

Re: Code Review of Caesar-Cipher

chrysaetos99

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 <[hidden email]> wrote:
Send Beginners mailing list submissions to
        [hidden email]

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
        [hidden email]

You can reach the person managing the list at
        [hidden email]

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 <[hidden email]>
To: [hidden email]
Subject: [Haskell-beginners] Code Review of Caesar-Cipher
Message-ID: <[hidden email]>
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 <[hidden email]>
To: The Haskell-Beginners Mailing List - Discussion of primarily
        beginner-level topics related to Haskell <[hidden email]>
Subject: Re: [Haskell-beginners] Code Review of Caesar-Cipher
Message-ID: <[hidden email]>
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 <[hidden email]> 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
> [hidden email]
> 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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners


------------------------------

End of Beginners Digest, Vol 143, Issue 5
*****************************************

_______________________________________________
Beginners mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners

_______________________________________________
Beginners mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners

caesar.hs (837 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Code Review of Caesar-Cipher

Daniel van de Ghinste (Lord_Luvat)
Hi,

Happy to help so far. I see your question in the comments of the code, asking why ‘flip' is needed. 

isAlphabetic :: Char -> Bool
isAlphabetic = flip elem alphabet

Basically, it’s needed by elem here. Let’s look at the signature of elem

Prelude> :t elem
elem :: (Eq a, Foldable t) => a -> t a -> Bool

Lists [ ] qualify as a Foldable t, so in the type signature let’s replace the ’t a’ with ‘[a]’ for clarity for you.

elem :: Eq a => a ->  [a] -> Bool

So, elem takes an ‘a’ as the first parameter and a 'list of a’ as the second parameter and returns a boolean. But we already have our list, and we won’t have the character we want to check until later! So, flip let’s switch that around! That way we can give elem the second parameter  first, so it becomes:

Prelude> :t (flip elem)
(flip elem) :: (Eq a, Foldable t) => t a -> a -> Bool

Or to rewrite it like we did above so it’s easier to read.

(flip elem) :: Eq a => [a] -> a -> Bool

If you look at the function ‘flip’ you’ll see this.

Prelude> :t flip
flip :: (a -> b -> c) -> b -> a -> c

As a beginner it might be easier for you to read if I make this small change.

flip :: (a -> b -> c) -> b -> (a -> c)

So flip takes a function which takes an ‘a’ and takes a ‘b’ and returns a ‘c’,
 it takes a ‘b’, puts that ‘b’ into the function you supplied,
 and then returns to you a function which takes an ‘a’ and returns a ‘c’.

So, if you don’t have flip in there
 elem alphabet
Then you are trying to give a list to elem in the position of it’s first parameter.

Does that help? 

Best regards,
Daniel van de Ghinste


On 21 May 2020, at 08:36, chrysaetos99 <[hidden email]> wrote:

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 <[hidden email]> wrote:
Send Beginners mailing list submissions to
        [hidden email]

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
        [hidden email]

You can reach the person managing the list at
        [hidden email]

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 <[hidden email]>
To: [hidden email]
Subject: [Haskell-beginners] Code Review of Caesar-Cipher
Message-ID: <[hidden email]>
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 <[hidden email]>
To: The Haskell-Beginners Mailing List - Discussion of primarily
        beginner-level topics related to Haskell <[hidden email]>
Subject: Re: [Haskell-beginners] Code Review of Caesar-Cipher
Message-ID: <[hidden email]>
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 <[hidden email]> 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
> [hidden email]
> 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
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners


------------------------------

End of Beginners Digest, Vol 143, Issue 5
*****************************************

_______________________________________________
Beginners mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners
<caesar.hs>_______________________________________________
Beginners mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners


_______________________________________________
Beginners mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/beginners