patternpythonMinor
Python Caesar Cipher
Viewed 0 times
caesarpythoncipher
Problem
I am a student doing their AS Computing course and I have created a piece of code in which a message can be typed and then scrambled by moving letters along the alphabet. I was wondering if anyone knew any way in which this could be improved.
while(True):
message = input("What would you like the message to say? ").lower()
newmessage = ""
for i in range(0,26):
for letter in message:
decimal = (ord(letter))
decimal = decimal + i
if decimal > 122:
decimal = decimal -26
elif decimal < 97:
decimal = decimal + 26
newmessage += (chr(decimal))
print (newmessage)
newmessage = ""Solution
Here are some comments to your code, which will be followed by an alternate implementation:
Algorithm considerations
If keeping to your format of not using functions, there are still some issues related to your algorithmic choices:
Adhering to my style and algorithmic considerations we arrive at this code:
Within the rotation block there is a commented line which could replace all of the other code, but it is not so easy to read and understand as the uncommented code.
Alternate implementation with functions
In general I would rather use functions to do stuff like this, and I would encapsulate my entire program within a if __name__ == '__main__':` block to allow for module execution. That is that the script could be imported as a module, and you could call the different functions to add caesar-cipher rotation to other programs of yours.
One version of this could be like this:
This uses string.ascii_lowercase for the set of lowercase letters and a positional approach for rotating a letter a given approach. The methods could be extended using default parameters to allow for other characters sets, but that is left as an exercise to the reader.
Furthermore I've presented two functions here one for rotating the entire string, and one for rotating a single character. These could have been combined, but I like the single responsibility principle of letting one functi
- Use functions to do the rotation – If you have learned about functions I would strongly suggest to use functions to do the actual rotation. You call it with the string to rotate and rotation offset, and you'll get the rotated string in return.
- Avoid magic numbers – In your code you use magic numbers like 122 and 97, your code looks cleaner if you replace these by what they actually are:
ord('z')andord('a'). Alternatively use constants likeALPHABET_LENGTH = 26, but I'll come back to this one
- Don't hide reserved words or modules – decimal is a module in Python, and to use that as a variable name is considered bad practice.
- Naming scheme dictates to use
snake_case– Instead ofnewmessageit is recommended to usenew_messageand similar. This reads more easy.
- Be consistent on indentation – You use a varying amount of spaces to indent your code, please be consistent and use 4 spaces. The previous and this item is related to conforming to the PEP8 guidelines. Please read and follow these guidelines, as that is quite common when using Python.
Algorithm considerations
If keeping to your format of not using functions, there are still some issues related to your algorithmic choices:
- What about letters not in the lowercase alphabet? – You don't handle punctuation chracters and non-alpha characters so these would leave somewhat strange behaviour
- You don't have a means to exit your code – You are essential having an eternal loop, leaving you to kill the program execution as only means of leaving it
- Range checks can be joined in Python – Instead of doing one lower than and one larger than you could do
97
- range()
default to start at 0 – That isrange(0, 26)andrange(26)is the same, and the latter is preferred for clarity
- You don't need to do (expr)
, unless you want line continuation – It is better to writenew_message += chr(decimal)without the extra parentheses. You could use them if you want to spread the expression within over multiple lines, but normally keep the expression without.
Adhering to my style and algorithmic considerations we arrive at this code:
ALPHABET_LENGTH = 26
while(True):
message = input("What would you like the message to say? ").lower()
if message == "quit":
break
for i in range(ALPHABET_LENGTH):
new_message = ""
for letter in message:
if 'a' <= letter <= 'z':
char_pos = ord(letter) - ord('a')
# Rotate by i, and make sure we're within alphabet
char_pos = (char_pos + i) % ALPHABET_LENGTH
new_message += chr(char_pos + ord('a'))
# new_message += chr((ord(letter) - ord('a') + i) % ALPHABET_LENGTH + ord('a'))
else:
new_message += letter
print (new_message)Within the rotation block there is a commented line which could replace all of the other code, but it is not so easy to read and understand as the uncommented code.
Alternate implementation with functions
In general I would rather use functions to do stuff like this, and I would encapsulate my entire program within a if __name__ == '__main__':` block to allow for module execution. That is that the script could be imported as a module, and you could call the different functions to add caesar-cipher rotation to other programs of yours.
One version of this could be like this:
from string import ascii_lowercase
def rotate_lowercase_char(char, offset):
"""Return lowercase rotated by characters.
If is a lowercase character rotate it, or else return
it unchanged allowing for non-alpha characters to be present."""
if not char in ascii_lowercase:
return char
else:
position = ascii_lowercase.index(char)
position = (position + offset) % len(ascii_lowercase)
return ascii_lowercase[position]
def rotate_lowercase_text(text, offset):
"""Return rotated by characters."""
return ''.join(rotate_lowercase_char(letter, offset) for letter in text)
def main():
while(True):
message = input("What would you like the message to say? ").lower()
if message == "quit":
break
for i in range(len(ascii_lowercase)):
print("Offset: {}, Rotated: {}".format(i, rotate_lowercase_text(message, i)))
if __name__ == '__main__':
main()This uses string.ascii_lowercase for the set of lowercase letters and a positional approach for rotating a letter a given approach. The methods could be extended using default parameters to allow for other characters sets, but that is left as an exercise to the reader.
Furthermore I've presented two functions here one for rotating the entire string, and one for rotating a single character. These could have been combined, but I like the single responsibility principle of letting one functi
Code Snippets
ALPHABET_LENGTH = 26
while(True):
message = input("What would you like the message to say? ").lower()
if message == "quit":
break
for i in range(ALPHABET_LENGTH):
new_message = ""
for letter in message:
if 'a' <= letter <= 'z':
char_pos = ord(letter) - ord('a')
# Rotate by i, and make sure we're within alphabet
char_pos = (char_pos + i) % ALPHABET_LENGTH
new_message += chr(char_pos + ord('a'))
# new_message += chr((ord(letter) - ord('a') + i) % ALPHABET_LENGTH + ord('a'))
else:
new_message += letter
print (new_message)from string import ascii_lowercase
def rotate_lowercase_char(char, offset):
"""Return lowercase <char> rotated by <offset> characters.
If <char> is a lowercase character rotate it, or else return
it unchanged allowing for non-alpha characters to be present."""
if not char in ascii_lowercase:
return char
else:
position = ascii_lowercase.index(char)
position = (position + offset) % len(ascii_lowercase)
return ascii_lowercase[position]
def rotate_lowercase_text(text, offset):
"""Return <text> rotated by <offset> characters."""
return ''.join(rotate_lowercase_char(letter, offset) for letter in text)
def main():
while(True):
message = input("What would you like the message to say? ").lower()
if message == "quit":
break
for i in range(len(ascii_lowercase)):
print("Offset: {}, Rotated: {}".format(i, rotate_lowercase_text(message, i)))
if __name__ == '__main__':
main()Context
StackExchange Code Review Q#121435, answer score: 3
Revisions (0)
No revisions yet.