I would greatly appreciate your feedback on my first ever Python project! :D
Basically I am coding a Caesar Cipher and I think its pretty terribly 'optimised / efficient' if you know what I mean, this is because I copied and pasted the encrypt() method for the decrypt() method and the only thing I changes was instead of rotating the numbers more, I rotated them less. This is what I'm talking about:
newPosition = (abc.find(letter) - key) % 26
^^ Instead of having a + (plus) I made it a - (minus) ^^
Is there a way I can sort of call the encrypt() method at just the newPosition line? Or what I did was correct and it doesn't need fixing (which I highly doubt)
** Please do take in mind that I do not have much knowledge in Python (if any at all) since I just started today so don't blow my brain up with some super complex code. THANK YOU!!! **
abc = 'abcdefghijklmnopqrstuvwxyz'
def main():
message = input("Would you like to encrypt or decrypt a word?")
if message.lower() == "encrypt":
encrypt()
elif message.lower() == "decrypt":
decrypt()
else:
print("You must enter either 'encrypt' or 'decrypt'.")
main()
def encrypt():
message = input("Enter a message to encrypt: ")
message = message.lower()
key = int(input("What number would you like for your key value?"))
cipherText = ""
for letter in message:
if letter in abc:
newPosition = (abc.find(letter) + key) % 26
cipherText += abc[newPosition]
else:
cipherText += letter
print(cipherText)
return cipherText
def decrypt():
message = input("Enter a message to decrypt: ")
message = message.lower()
key = int(input("What number would you like for your key value?"))
cipherText = ""
for letter in message:
if letter in abc:
newPosition = (abc.find(letter) - key) % 26
cipherText += abc[newPosition]
else:
cipherText += letter
print(cipherText)
return cipherText
main()
In general, str.find is bad performance-wise. It's O(n) complexity, which isn't awful, but you rarely actually need it. In this case you can use ord to convert each letter to its ordinal, then subtract ord('a') to get 0-25 instead of 97-122.
This is particularly useful, because you can then use chr to convert back without needing a lookup.
for letter in message:
if letter in string.ascii_lowercase: # same as "abcdef..z"
new_position = ((ord(letter) - ord('a') + key) % 26) + ord('a')
new_ch = chr(new_position)
ciphertext += new_ch
Note also that concatenating strings with += isn't as fast as something like str.join.
new_letters = [chr(((ord(letter) - ord('a') + key) % 26) + ord('a')) if letter in ascii_lowercase else letter for letter in message]
ciphertext = "".join(new_letters)
And since that chr(((ord(letter) - ord('a') + key) % 26) + ord('a')) is so ugly, I'd refactor that into a function.
def rotate(letter, key=0):
c_pos = ord(letter) - ord('a')
rotated = c_pos + key
modded = rotated % 26
final_pos = modded + ord('a')
return chr(final_pos)
new_letters = [rotate(c, key) if c in string.ascii_lowercase else c for c in letters]
ciphertext = "".join(new_letters)
A point of maintainability: it's easier to write testably good code if you separate your inputs from your results. Right now you'd have to do some monkey patching of stdin to write unit tests for any of your functions, but if you move your requests for user input into main and out of their respective function, that gets much easier.
def main():
message = input("What's the message to encrypt/decrypt? ")
key = int(input("What number would you like for your key value? "))
choice = input("Choose: encrypt or decrypt. ")
if choice == "encrypt":
result = encrypt(message, key)
elif choice == "decrypt":
result = decrypt(message, key)
else:
# something here about a bad user input.
In fact, when you consider the Caesar Cipher is reversible by flipping the sign of the key, you can simply do:
if choice == "encrypt":
result = encrypt(message, key)
elif choice == "decrypt":
result = encrypt(message, key * (-1))
and not write a decrypt function at all!
Collected from the Internet
Please contact [email protected] to delete if infringement.
Comments