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.")
  • 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. – TemporalWolf Oct 16 '17 at 17:45
  • 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. – Oscar Smith Oct 16 '17 at 17:49
  • @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. – TemporalWolf Oct 16 '17 at 18:26
  • Your code is unnecessary complicated – Billal Begueradj Oct 19 '17 at 15:04
  • Here is my solution that gets rid of the duplicated code of your two main functions. – Billal Begueradj Nov 12 '17 at 18:24
up vote 3 down vote accepted

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.")
  • 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. – Roland Illig Oct 17 '17 at 5:21
  • makes sense, updated – Oscar Smith Oct 17 '17 at 5:29
  • What do the numbers 25 and 91 in your answer mean? ;) – Roland Illig Oct 17 '17 at 5:37
  • 1
    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 Oct 17 '17 at 5:50
  • 1
    Or write it as ord('A') + 26, for symmetry with the encrypt function. – Roland Illig Oct 17 '17 at 5:52

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.

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.