Reading and writing data for Caesar cipher

Question

I am struggling with commenting and variable naming. My teacher says my comments need to be more explanatory and explicit. He says my variable names are also sometimes confusing. I was just wondering whether you could go through my code, see whether you are able to understand the comments and whether the variables names are appropriate. And of course, suggest improvements where you feel they are needed.

#Caesar Cipher

#Function that reads and writes data to the file, and calls the encryption and decryption methods.
#It isolates the key and choice of the user, and then calls the appropriate function.
def isolate():
    #Open input file in read mode
    fin = open('Input3.txt', 'r')
    #Open output file in write mode
    fout = open('Out1.txt', 'w')
    #Create a list that holds each line of the file as an element of the list
    container = fin.readlines()

    #For loop that goes through each line of the file (contianed in list) and isolates the choice and key of the user
    #Then it removes the key and choice from the split input list and calls the appropriate method.
    for i in container:
        #Each element of the list becomes a list (words) with all spaces and '\n' removed
        words = i.split()
        #The key of the user is always the last element of the split() list.
        key = int(words[len(words) - 1])
        #The choice of the user is always the second last element of the split() list.
        choice = words[len(words) - 2]
        #Remove the key and choice from the message
        words.remove(str(key))
        #Remove the choice of the user from the message for encryption/decryption.
        words.remove(choice)

        #Join together all the elements of the message in a string
        message = ' '.join(words)
        message = message.upper()

        #If the user wishes to encrypt, call the encrypt method. This method will return the encrypted message as a sting, which can then be written to the output file.
        if choice == 'e':
            #Write the newly encrypted message to the output file.
            fout.write(encrypt(message, key) + '\n')
        #If the user wishes to decrypt, call the decrypt method. This method returns the decrypted message as a string, which can then be written to the output file.
        else:
            fout.write(decrypt(message, key) + '\n')

    #Close the file to make sure data is written properly to the file.
    fout.close()

#Encryption method, which takes two parameters, the users message and key, and returns the newly encrypted message.
def encrypt(message, key):
    #Empty string that will contain the encrypted message.
    encrypted_message = ""
    #Alphabet string, which contains all the letters of the alphabet at their index locations.
    alpha = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'

    #For loop that goes through each character of the users message and, if a space, adds it straight to the encrypted message or encrypts it through modular division.
    for i in message:
        #If the character is a space, add it to the final message.
        if i == ' ':
            #Concatenate the space to the final message
            encrypted_message += ' '
        #If the character is not a space, determine the final locatin out of index range, % by 26 and re-add the character at this index to the encrypted_message.
        else:
            #Determine the final location of the character (out of index range)
            location = key + alpha.index(i)
            #% this location by 26 to determine the location within the proper index.
            location %= 26
            #Finally, concatenate the letter at this location to the final message.
            encrypted_message += alpha[location]

    #When the function is called, have it return the final encrypted message.
    return encrypted_message


#Decryption method that takes two parameters, the users message and key, and returns the newly decrypted message.
def decrypt(message, key):
    #Create a string that reverses the alphabet, since we are moving backwards when we decrypt.
    reverse_alpha = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'[::-1]
    #Empty string that will hold the decrypted message.
    decrypted_message = ""

    #For loop that runs through each element of the users message and, if space, re adds it to the message, or encrypts using % division and concatenates to final message.
    for i in message:
        #If the element is a space, do not encrypt; concatenate it to the final message.
        if i == ' ':
            #Concatenate the space to the final message.
            decrypted_message += ' '
        #If the element is not a space, encrypt the letter.
        else:
            #Determine the final location out of index range of the encrypted character, and store it in the variable 'location'
            location = key + reverse_alpha.index(i)
            #% this position by 26 to determine the index location in range of the letter.
            location %= 26
            #Finally, add the letter at this location to the decrypted message.
            decrypted_message += reverse_alpha[location]

    #Converts the decrypted message into lowercase form.
    decrypted_message = decrypted_message.lower()

    #When the function is called, have it 'give back' the decrypted message so that it can be written to the output file.
    return decrypted_message

#Call the function isolate5(), which initiates the program.
isolate()

Answer 1

Commenting every single line is excessive by most programmers' norms. There is a coding style called literate programming that heavily emphasizes comments, but even in literate programming, commenting every line would be frowned upon.

The most important kind of comment in Python is the docstring, and you didn't write any.

Many of your comments just repeat self-explanatory code. For example, this comment does nothing but double the amount of text a programmer has to read:

#Open input file in read mode
fin = open('Input3.txt', 'r')

If you really had to guide a novice, you could make the comment less obtrusive by relocating it to the end of the line:

fin = open('Input3.txt', 'r')                   # 'r' = read-only

It's usually easy to see what each line of code does individually. However, comments can be very useful in providing the big picture. You would also be able to do a better job of describing intentions by explaining several lines of code at once.

Improving the naming and comments without changing the code at all:

def run_caesar_cipher_batch():
    """Processes Input3.txt to generate Out1.txt.
    Input3.txt contains input lines of the form
        The quick brown dog         e 3
        VJG SWKEM DTQYP FQI         d 3

    The last two words of each line are:
    'e' or 'd', indicating the choice to encrypt/decrypt, and
    an integer, which acts as the Caesar cipher key.

    Within each line, all whitespace, even multiple consecutive
    whitespace characters, is treated as a single space.
    """
    fin = open('Input3.txt', 'r')
    fout = open('Out1.txt', 'w')
    input_lines = fin.readlines()

    for line in input_lines:
        # List of words, with whitespace and trailing '\n' removed
        words = line.split()

        # Interpret and remove the last two words
        key = int(words[len(words) - 1])
        operation = words[len(words) - 2]
        words.remove(str(key))
        words.remove(operation)

        # Re-join the words of the message. The message may differ from
        # the original if there is consecutive whitespace.
        message = ' '.join(words)
        message = message.upper()

        # Encrypt or decrypt to fout
        if operation == 'e':
            fout.write(encrypt(message, key) + '\n')
        else:
            fout.write(decrypt(message, key) + '\n')

    fout.close()

Answer 2

You'll find that there are many attitudes, but the type of commenting you've shown here is largely only helpful as a language learner. When I look at the code, already familiar with the language, the comments are way too explicit. You've essentially got comments repeating what every line of code does, often overshadowing the more important question of why.

I already know that a def line defines a function, what the arguments of open mean, I can see the number of arguments in the code, and I know that an else block has the opposite condition to the if. It's even easier, knowing Python's slice conventions, to read words[-1] as the last word than something involving function calls (words[len(words) - 1]). Also, the descriptions of functions really belong in docstrings, so I can look them up in the interpreter.

As for naming, there is very little to complain about. fin and fout could be expanded, mostly to avoid confusion with the word fin, and the iteration name i is a holdover from the old days of interpreters that only handle single letter variables; nowadays we use it to avoid tedious typing, but that's not a good reason when it's only typed twice. A descriptive name such as line would be better. The one bugbear is isolate, because that doesn't describe the procedure at all; it seems to do batch processing of files, generating a subroutine call for each line. I suppose that iteration processes lines in isolation, but it's really not clear from the call what is manipulated or in what way.

What I'm missing in the comments is an example of how the input and output data in files would look. The actual column format (looks like message e|d key, where key is an integer, e means encrypt, and message may contain spaces) isn't easily read from the parsing code. We also run into logic mistakes when you remove choice and key from words; you really ought to use key = int(words.pop()), such that it's impossible to confuse which item you're removing. Converting to and from int, and searching by value rather than place, will break for some lines, e.g. secret e message e 003.

The last comment demonstrates the biggest problem with comments that describe what the code does, rather than what it is intended to do. They invariably get out of sync, and at that point only cause confusion.

The code itself could be reduced considerably using Python's high level libraries (for instance, str.translate and str.rsplit) but that is quite separate from the naming and commenting you asked opinions on.

Answer 3

You are commenting far too much, to the point that it obscures the program. For example,

# When the function is called, have it 'give back' the decrypted message so that it can be written to the output file.
return decrypted_message

Surely return decrypted_message should be self-explanatory?

In fact, this "explanation" is almost deceptive - the function doesn't actually know (or need to know) where the output is going - it decrypts a message, it doesn't care whether the message is going to be printed to the screen or sent in an email or thrown away.

You are also commenting at the wrong level - explaining what each line does instead of why it does it. For example

#Converts the decrypted message into lowercase form.
decrypted_message = decrypted_message.lower()

That's nice, but why should the message be lowercase?

The whole thing could be much more succinct:

# Caesar Cipher
#     Encrypt or decrypt using a shift-by-N cipher

ORD_A = ord('a')

def caesar_cipher(msg, n):
    """
    Return an alphanumeric string with each letter incremented N characters
    """
    return ''.join(' ' if ch==' ' else chr((n + ord(ch) - ORD_A) % 26 + ORD_A) for ch in msg)

def process_file(infname, outfname):
    """
    Read a text file
    Encrypt or decrypt each line
    Write the result to a text file
    """
    with open(infname) as inf, open(outfname, 'w') as outf:
        for line in inf:
            # Each line is of the form 'text to encrypt [op] [n]'
            # where op is 'd' to decrypt or 'e' to encrypt
            # and n is the number of characters to shift by
            msg, op, n = line.rsplit(None, 2)

            # Decrypting by N is the same as encrypting by -N
            n   = int(n) * (1 if op=='e' else -1)

            # Encrypt and save the result
            msg = caesar_cipher(msg.lower(), n)
            outf.write(msg + '\n')

if __name__=="__main__":
    inf  = raw_input('Name of input file? ')
    outf = raw_input('Name of output file? ')
    process_file(inf, outf)

Answer 4

The code in the OP is extremely over-commented. (No offense)

The other answers have covered all the bases, very well, but there is one thing I want to emphasise from Yann Vernier's answer:

The last comment demonstrates the biggest problem with comments that describe what the code does, rather than what it is intended to do. They invariably get out of sync, and at that point only cause confusion.

This is the biggest problem with over-commented code, in general. It's a serious concern:

• Comments are just like anything else in a code-base, they have to be maintained.
• Out of synch comments are not just worthless; they're dangerous.

Personally, I think under-commenting is preferable to over-commenting, any day of the week. An assignment is one thing; it doesn't change when you're done. But, most code lives on and a variety of people change it, each one having to digest and maintain both the code and the comments.

Please be selective when commenting.

Answer 5

I understand that you welcome suggestions other than regarding comments:

And of course, suggest improvements where you feel they are needed.

Your encode and decode functions could be rewritten to better utilize Python facilities:

from string import ascii_lowercase

def caesar_cipher(message, n, op='e', alphabet=ascii_lowercase):
  """
  Encode or decode `message` using Caesar cipher with shift `n`.

  Each letter in `message` gets replaced by a letter `n` positions
  down the `alphabet` if encoding (`op=='e'`), or up if decoding (`op=='d'`).

  Characters not in `alphabet` remain unchanged.
  """
  if op=='d': n = -n
  shift = dict(zip(alphabet, alphabet[n:] + alphabet[:n]))
  return ''.join(shift.get(c, c) for c in message)

Slice notation makes it easy to cyclically rotate lists, which is exactly what we need for Caesar cipher. It works perfectly for decoding also, because a[-i:] means a[len(a)-i:] in Python.

We build a dictionary for the cipher mapping by zipping together respective positions of the original and shifted lists.

dict has the handy get method that lets us provide a default value to return if the key is not present in the dict. This way, we don't need to check if the character is a space and return a space if it is, instead we use it as the default value. For free this handles other characters like numbers or punctuation.

Instead of a loop, use a generator expression and pass it as argument to ''.join().

By making alphabet a parameter, we allow the use of more characters for encoding with the cipher.

Finally, I included a docstring that documents the method, to address your main concern of commenting the code.

Note that if I followed the commenting style of the code from your original post, I would have to place all the explanation of how the code works in this answer as comments in the code. However, this is not what comments are for. When writing code for nondidactic purposes, you assume the person reading the code either knows the language very well, or can check the documentation for some particular parts. So instead of commenting how shift.get(c, c) works, I assume the reader knows it or will see the documentation of dict.get. I left the comment saying that characters not in alphabet remain unchanged, but I don't explain how that happens, as I expect it is easy enough to infer that from the code and documentation.

Things that warrant an explanation in a comment are things that are not offered by the language or libraries we use. Comments should be used for the logic we create.

---

Alternatively, the caeser_cipher could use str.translate as suggested by Yann Vernier, with str.maketrans like this:

message.translate(str.maketrans(alphabet, alphabet[n:]+alphabet[:n]))

Which makes for an even better implementation.