3
\$\begingroup\$

I'm a relatively new programmer. I've made a simple Vigenere cipher program. It takes three arguments and acts on a file. I have made some of the steps more "explict" for myself by using more lists than I need to, rather than applying multiple "transformations" at a time. I would appreciate feedback on how this code would be written differently by people who know more than I do.

#!/usr/bin/env python3

# vigenere.py - This program has two modes, encrypt and decrypt. It takes
# three arguments: the mode('encrypt' or 'decrypt'), a keyword, and a
# filename to act upon. It is designed to work with lowercase letters.

from sys import argv
from itertools import cycle

# User specifies a mode, a key, and a file with argv arguments
def start():
    if len(argv) > 1:
        mode = argv[1]
        key = argv[2]
        plaintextFilename = argv[3]
    else:
        print('Please supply mode, key, and file as arguments.')
        exit()

    # Start the mode selected
    if mode == 'encrypt':
        encryptMode()
    elif mode == 'decrypt':
        decryptMode()
    else:
        print('Please supply \'encrypt\' or \'decrypt\' mode.')
        exit()


# Encryption Mode
def encryptMode():

    # Open the alpha plaintext file as an object
    alphaPlaintextFileObj = open(argv[3])

    # Create the ordinal plaintext data structure
    ordinalPlaintext = []

    # Populate the ordinal plaintext data structure
    for c in alphaPlaintextFileObj.read():
        if c == ' ':
            ordinalPlaintext.append(' ')
        else:
            o = ord(c) - 65
            ordinalPlaintext.append(o)

    # Create an ordinal ciphertext data structure
    ordinalCiphertext = []

    # Turn the key into an ordinal key where a = 1, etc.
    ordinalKey = []
    key = argv[2]
    for c in key:
        n = ord(c) - 96
        ordinalKey.append(n)

    # Populate the ordinalCiphertext structure with numbers shifted using the
    # ordinal key.
    for k, p in zip(cycle(ordinalKey), ordinalPlaintext):
        if p == ' ':
            ordinalCiphertext.append(' ')
        else:
            c = (k + p) % 25
            ordinalCiphertext.append(c)

    # Create the alpha ciphertext file
    alphaCiphertextFilename = argv[3] + '_encrypted'
    alphaCiphertextFileObj = open(alphaCiphertextFilename, 'w')

    # Populate the alpha ciphertext file
    for c in ordinalCiphertext:
        if c == ' ':
            alphaCiphertextFileObj.write(' ')
        else:
            l = chr(int(c) + 65)
            alphaCiphertextFileObj.write(l)

    # Save and close the plaintext and ciphertext files.
    alphaPlaintextFileObj.close()
    alphaCiphertextFileObj.close()

    # Print a message telling the user the operation is complete.
    print(f'{argv[3]} encrypted as {alphaCiphertextFilename}')

# Decryption Mode
def decryptMode():
    # Open the alpha ciphertext file as an object
    alphaCiphertextFileObj = open(argv[3])

    # Create the ordinal ciphertext data structure
    ordinalCiphertext = []

    # Populate the ordinal ciphertext data structure
    for c in alphaCiphertextFileObj.read():
        if c == ' ':
            ordinalCiphertext.append(' ')
        else:
            o = ord(c) - 97
            ordinalCiphertext.append(o)

    # Create the ordinal key
    ordinalKey = []
    key = argv[2]
    for c in key:
        n = ord(c) - 96
        ordinalKey.append(n)

    #Create the ordinal plaintext data structure
    ordinalPlaintext = []

    # Populate the ordinal plaintext data structure with the modular
    # difference of the ordinal ciphertext and the ordinal key
    for k, c in zip(cycle(ordinalKey), ordinalCiphertext):
        if c == ' ':
            ordinalPlaintext.append(' ')
        else:
            p = (c - k) % 25
            ordinalPlaintext.append(p)

    # Create the alpha plaintext file
    alphaPlaintextFilename = argv[3] + '_decrypted'
    alphaPlaintextFileObj = open(alphaPlaintextFilename, 'w')

    # Convert the ordinal plaintext to an alpha plaintext file,
    # 'filename_decrypted'
    for p in ordinalPlaintext:
        if p == ' ':
            alphaPlaintextFileObj.write(' ')
        else:
            l = chr(int(p) + 97)
            alphaPlaintextFileObj.write(l)

    # Save and close the ciphertext and plaintext files
    alphaCiphertextFileObj.close()
    alphaPlaintextFileObj.close()

    # Print a message telling the user the operation is complete
    print(f'{argv[3]} decrypted as {alphaPlaintextFilename}')

start()
\$\endgroup\$
3
\$\begingroup\$
def start():

I'd call this function main as that's what it is generally called.

if mode == 'encrypt':
    encryptMode()
elif mode == 'decrypt':
    decryptMode()

Why not call this encrypt and decrypt? The methods to actually perform the encryption / decryption after all; you're not setting a mode.

alphaPlaintextFileObj = open(argv[3])

It seems to me that file handling can be perfectly split from the encrypt function, especially if you read in all the data before encryption happens anyway.

ordinalPlaintext = []

Why would you first convert the entire plaintext / ciphertext to ordinals? This can be done on a per-character base, preferably using a separate method. Then it also becomes easier to skip space and such, which you now have to handle twice.

Conversion to ordinals - or more precisely, indices within the Vigenere alphabet - is of course exactly what is needed, so that's OK.

o = ord(c) - 65

65 is an unexplained magic number, why not use ord('a') instead or use a constant with that value?

n = ord(c) - 96

Why is A a 1? What about Z in that case? And why do we suddenly use the uppercase character set?

for k, p in zip(cycle(ordinalKey), ordinalPlaintext):

Now this I like, it is very clear what is done here, and it is good use of Python specific functionality.

c = (k + p) % 25

Wrong! You always perform a modular calculation with the same size as the alphabet. This might work as well (if you forget about the Z) but it's not Vigenere as it was written down a long time ago.

alphaPlaintextFileObj.close()

Always close files as soon as they are not necessary any more. You already read all of the plaintext, no need to keep that file handle around.


What I'm missing is validation that the contents of the plaintext consist of characters that are out of range, and a way of handling those. The same thing goes for the key, which should consist of all uppercase characters, but lowercase characters are used without issue.


Furthermore, if you take a good look, then decryption is the same as encryption, except for p = (c - k) % 25 and - of course - the file handling. Now the file reading and writing should not be in either method, so let's exclude that. That leaves us with that single assignment / expression. Of that, only the - sign is really different.

This is why most people will write a single "private" _crypt method that simply takes an integer of 1 for encryption and -1 for decryption. Then the expression becomes (charIndex + direction * keyIndex) % alphabetSize.

Currently you are violating the "DRY principle": don't repeat yourself.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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