Vigenere Cipher - make code more concise?

Question

I'm a beginner in programming overall, started about a month ago and so I'm still trying to fully grasph the concept of OOP, for instance. Here I used a class for the encryption and decryption processes, but everything else is in functions in the main part of the program (getting the cipher key, user message etc). What do you guys think? Is my implementation good? How could I improve it?

# Vigenere Cipher program
class Vigenere:
 """Vigenere Cipher program."""
 def __init__(self, message, key):
 self.message = message
 self.key = key
 def __str__(self):
 print("Vigenere Cipher program.")
 def __encrypt(self):
 encrypted = ""
 i = 0
 for letter in self.message:
 if ord(letter) + self.key[i] > 90:
 new_key = ord(letter) + self.key[i] - 90
 new_letter = chr(64 + new_key)
 encrypted += new_letter
 i += 1
 if len(self.key) == i:
 i = 0
 else:
 encrypted += (chr(ord(letter) + self.key[i]))
 i += 1
 if len(self.key) == i:
 i = 0
 return encrypted
 def __decrypt(self):
 decrypted = ""
 i = 0
 for letter in self.message:
 if ord(letter) - self.key[i] < 65:
 new_key = 65 - (ord(letter) - self.key[i])
 new_letter = chr(91 - new_key)
 decrypted += new_letter
 i += 1
 if len(self.key) == i:
 i = 0
 else:
 decrypted += chr(ord(letter) - self.key[i])
 i += 1
 if len(self.key) == i:
 i = 0
 return decrypted
def get_message():
 userMessage = ""
 final_message = ""
 flag = True
 while flag:
 userMessage = input("\nYour message: ")
 if len(userMessage) < 1:
 print("\nYou must enter something!")
 continue
 userMessage = userMessage.replace(" ", "")
 for i in range(len(userMessage)):
 if not userMessage[i].isalpha():
 continue
 else:
 final_message += userMessage[i]
 if len(final_message) < 1:
 continue
 else:
 flag = False
 return final_message.upper()
def get_key():
 userKey = ""
 keys = []
 final_keys = []
 flag = True
 while flag:
 userKey = input("\nYour key: ")
 if len(userKey) < 1:
 print("You must enter something!")
 continue
 userKey = userKey.replace(" ", "")
 for i in range(len(userKey)):
 if not userKey[i].isalpha():
 continue
 else:
 keys.append(ord(userKey[i].upper()))
 final_keys.append(keys[i] - 64)
 if len(final_keys) < 1:
 continue
 flag = False
 return final_keys
def encrypt_or_decrypt():
 userChoice = ""
 flag = True
 while flag:
 userChoice = input("\nEncrypt or decrypt? (E/D): ")
 if userChoice not in ("e", "E", "d", "D"):
 continue
 else:
 flag = False
 return userChoice
if __name__ == "__main__":
 flag = True
 while flag:
 userChoice = encrypt_or_decrypt()
 message = get_message()
 key = get_key()
 cipher = Vigenere(message, key)
 if userChoice in ("e", "E"):
 encrypted = cipher._Vigenere__encrypt()
 print(encrypted)
 else:
 decrypted = cipher._Vigenere__decrypt()
 print(decrypted)
 userInput = input("\nAgain? (Y/N): ")
 while userInput not in ("y", "Y", "n", "N"):
 userInput = input("\nAgain? (Y/N): ")
 if userInput in ("y", "Y"):
 continue
 elif userInput in ("n", "N"):
 print("Exiting.")
 flag = False
 input("\n\nPress the enter key to exit.")

• TemporalWolf: Decryption of key "AAAA" is equivalent to encryption via "YYYY": if you calculate the inverse key you can just def decrypt(key): return encrypt(inverse(key)) and save all the duplication you have from implementing the cryptography twice. You do need to write the key inversion, however.
• Oscar Smith: I thought about mentioning that in my answer, but honestly I think that calculating the inverse key takes about the same amount of code as the ~8 lines for duplicating the encryption. It's also arguably more clear.
• TemporalWolf: @OscarSmith key inversion is considerably simpler and makes it obvious the encryption is symmetric, but I agree it's debatable whether this is more or less clear.
• Billal BEGUERADJ: Your code is unnecessary complicated
• Billal BEGUERADJ: Here is my solution that gets rid of the duplicated code of your two main functions.

Answer 1

Overall, nice first post. Your code is clear, and follows pep8 quite well. WRT to you OOP style, I would make some changes. Firstly, encrypt and decrypt really shouldn't be private methods, as they will only be called from outside the class. I also probably would not store the message and key in a class instance, but rather have your methods be encrypt(self, message, key) and have decrypt do the same.

In terms of the cleanliness of your code, your encrypt and decrypt methods would be cleaner if you used itertools.cycle to keep track of the key. All these changes yield the following methods. Also, instead of subtracting 90, and adding 64, it would probably be easier to just subtract 25

def encrypt(self, message, key):
 encrypted = ""
 for letter, key_i in zip(message, itertools.cycle(key)):
 if ord(letter) + key_i > ord('Z'):
 encrypted += chr(ord(letter) + key_i - 26)
 else:
 encrypted += (chr(ord(letter) + key_i)
 return encrypted
def decrypt(self, message, key):
 decrypted = ""
 for letter, key_i in zip(message, itertools.cycle(key)):
 if ord(letter) - key_i < ord('A'):
 new_key = ord('A') - (ord(letter) - key_i)
 new_letter = chr(91 - new_key)
 decrypted += new_letter
 else:
 decrypted += chr(ord(letter) - key_i)
 return decrypted

One sign that these are good changes is that they simplify your driving code to

if __name__ == "__main__":
flag = True
while flag:
 userChoice = encrypt_or_decrypt()
 message = get_message()
 key = get_key()
 if userChoice in ("e", "E"):
 encrypted = Vigenere.encrypt(message, key)
 print(encrypted)
 else:
 decrypted = Vigenere.decrypt(message, key)
 print(decrypted)
 userInput = input("\nAgain? (Y/N): ")
 while userInput not in ("y", "Y", "n", "N"):
 userInput = input("\nAgain? (Y/N): ")
 if userInput in ("y", "Y"):
 continue
 elif userInput in ("n", "N"):
 print("Exiting.")
 flag = False
input("\n\nPress the enter key to exit.")

• Roland Illig: 25 is a magic number here, and probably a bug. It should be 26, which is the length of the alphabet. Also, 90 should be written as ord('Z'), so that the reader doesn't have to know the Unicode code charts by heart.
• Oscar Smith: makes sense, updated
• Roland Illig: What do the numbers 25 and 91 in your answer mean? ;)
• Roland Illig: Oh, I was not asking for myself, I was asking for the readers of the code. You should remove the number 25 from your answer and replace the 91 with ord('Z') + 1, to make the code consistent.
• Roland Illig: Or write it as ord('A') + 26, for symmetry with the encrypt function.

Answer 2

As far as OOP languages go, Python has a very weak sense of encapsulation.

The only thing we do have are putting a single _ in front of a method or variable name, to signal that it is an internal variable/method and should not be used unless within the class itself and maybe derived classes. This is a pure convention, but should be understood similarly to protected in other languages.

In addition to this, when we put two underscores in front__ of the name, this signals that the variable/method is what we would call private in other languages. For these variables, Python does some name wrangling, so for some object a = A(), a.__x becomes accessible only as a._A__x. This should already signal that you should not normally use this from outside the class, these variables are definitely not part of the public API.

So, to put it shortly, just rename your __encrypt and __decrypt methods to encrypt and decrypt.

This has the added advantage that if you have a different encryption class (which you seem to do, called Caesar), the interface does not change. With both you can do cipher.encrypt(...) and cipher.decrypt(...), so the only thing you have to change when changing the encryption engine, is swap the actual objects.