3
\$\begingroup\$

I'm newbie in Python and I wrote a tool called Caesar Encryption Tool. At the moment it only encrypts. It takes a plain text and a key from the user then encrypts the plain text using the key. How can this code be improved?

# This is version 1.0 Caesar Encryption Tool and it will be improved.
# This tool is written by Mahmoud Naguib.
# My facebook account is at : https:\\www.facebook.com\naguibarea
# This is letters table.
lettersTable = ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q',
            'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z']
# This line takes the user's key it's 3 by default.
key = int(input('Enter your key : '))
# This line takes the user's plain text and converts it
#  to uppercase to search for it in the English alphabet.
plainText = input('Enter your plain text : ').upper()
for letter in range(len(plainText)):
    # If there is a letter which isn't alphabet like numbers,
    #  special characters and spaces it will be appended to the list.
    if plainText[letter] not in lettersTable:
        lettersTable.append(plainText[letter])
    # This line locates the plain letter's numeric value in other words its    position.
    position = lettersTable.index(plainText[letter])
    # This line adds the key entered by the user to the plain letter's
    #  numeric value to reach the new encrypted letter.
    step = position + key
    # if the encrypted letter's numeric value is greater than 25
    #  then its new numeric value is the old numeric valuu %
    #  the number of English alphabet letters.
    if step > 25:
        step %= 26
    # If position is greater than 25 that means it's not an alphabet
    #  value and it will be printed without adding the key.
    if position > 25:
        step = position
    # This line locates the encrypted letter's numeric value in other words
    #  its position and prints the encrypted letter.
    # end is a keyword argument to combine all letters together.
    print(lettersTable[step], end='')
\$\endgroup\$
3
\$\begingroup\$

Welcome to Code Review.

I would be going through this code quickly.

  1. Read PEP 8. It is de facto standard for coding in python. Generally, we name the variables in lower_case_with_underscore, and any constant used as CONSTANT_UNDERSCORE.
  2. Use in-built functions. You are using your letters_table to find out whether the character is alphabet or not, use isalpha() instead. Similarly you can use ord and chr functions which would allow you to preserve case. Using all of these functions would remove the need of letters_table
  3. Keep consistent comments. Your comments indicated that key default value is 3. It is not, and if I leave input blank, it would raise error.
  4. Make your code modular. You can easily separate your I/O logic with implementation logic. (Use functions)

Since you indicate you want to improve this code yourself, I would refrain from posting refactored version of your code. (If you want me to as an example, I can do that).

Here are some more improvements which you could incorporate:

  1. Allow user an option to select between encryption and decryption mode. (with functions, this would be done by having an extra parameter with default value)
  2. Other encryption logic while maintaining the same I/O logic. (Would help you understand power of function modules)

Some Python code quality skills which might be handy in future:

  1. Write useful docstring (see examples in link). While for this program, your comments are more than enough, it is useful skill.
  2. Learn to write some assertion tests and then unit tests.
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer! I'll take this tips in mind when I learn more about Python and when I develop my code.You are absoulotely right. \$\endgroup\$ – Mahmud Muhammad Naguib Oct 10 '15 at 18:27
  • \$\begingroup\$ You are welcome. If you want, you can post another question with refactored code, with title "follow up" and CR community would help you with it \$\endgroup\$ – kushj Oct 11 '15 at 14:58
  • \$\begingroup\$ Note that the lower_case and UPPER_CASE styles are known (in the Python style guide at least) as lower_snake_case and UPPER_SNAKE_CASE. \$\endgroup\$ – SuperBiasedMan Oct 12 '15 at 9:35
2
\$\begingroup\$

Some notes about lettersTable. The other answer already addressed using isalpha, but if you wanted to test against a list of the characters then there's easier methods. You can actually use character in string perfectly fine, ie. if plainText[letter] in "ABCDEFGHIJKLMNOPQRSTUVWXYZ" will work. For certain cases strings can be treated like iterables and this is one of those cases. But also, you can use the string module to get an alphabet constant.

from string import ascii_uppercase as lettersTable

That line removes the need for having either a list or string defined in your case.

Your code is swimming in comments. You don't need to explain everything. For instance:

# This is letters table.

is self evident. Python's pretty readable, so anyone who could follow your script at all will know how to follow the variable name. More than 2/3 of the script is comments, and you'd actually make it more readable by removing them:

for letter in range(len(plainText)):

    # Add non alphabet characters to the list.
    if plainText[letter] not in lettersTable:
        lettersTable.append(plainText[letter])

    position = lettersTable.index(plainText[letter])
    step = position + key
    if step > 25:
        step %= 26

    # If it's not an alphabet value print without the key
    if position > 25:
        step = position

    print(lettersTable[step], end='')

You notice the lines I've left don't really explain what the code does. Documentation and prior knowledge can do that better. Especially when most lines here aren't hard to follow. Instead I leave comments that explain the more abstract ideas, like why you're appending characters to the lettersTable that weren't there before.

One thing that your comments didn't explain is why you're adding the non letter characters if you're then going to ignore them? It seems like you did it to keep index from raising ValueErrors. If that's the case, using a string is better because then you could use lettersTable.find(plainText[letter]). str.find() returns the index of the character or it will return -1. You can then just check if the result is -1 rather than adding the letter to your lettersTable just to be an exception.

    position = lettersTable.find(plainText[letter])
    # If it's not an alphabet value print without the key
    if position == -1:
        print (plainText[letter], end='')
    else:
        step = position + key
        if step > 25:
            step %= 26

Also you could use a better for loop. In Python you can loop directly over the elements of a string or list. This means you no longer need to keep accessing the index of plainText to get the value and it'll be much more readable.

for letter in plainText:

    position = lettersTable.find(letter)
    if position == -1:
        # If it's not an alphabet value print without the key
        print (letter, end='')
    else:
        step = position + key
        if step > 25:
            step %= 26

        print(lettersTable[step], end='')
\$\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.