3
\$\begingroup\$

I'm a beginner in Python. I wrote this code which encrypts and decrypts text using Caesar chipher. I wrote it in many days and optimized it many times. But how it can be improved more?

I've used lists because they are flexible and they serve my algorithm.

print('''# This is version 1.0 Caesar Encryption-Decryption Tool.
# This tool is written by Mahmoud Naguib and it will be improved.
# You can contact me at : https://www.facebook.com/naguibarea
-----------------------------------------------------------------''')
letters_table = ['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']


def caesar():
    # This line iterates through every letter in the string.
    for letter in range(len(text)):
        # If there is a letter which isn't an English alphabet.
        if not text[letter].isalpha():
            letters_table.append(text[letter])
        # This line locates the letter's numeric value in other words its position.
        position = letters_table.index(text[letter])
        # This line adds the key entered by the user to the letter's
        # numeric value to reach the new 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 step is lower than 0 then we need to add 26 to it to complete the decryption process.
        if step < 0:
            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(the letter will be printed as it is).
        if position > 25:
            step = position
        # This line locates the letter's numeric value(its position) and prints the new letter.
        # end is a keyword argument to combine all letters together.
        print(letters_table[step], end='')
    print('\n' + '-----------------------------------------------------------------')


while True:
    while True:
        # This line ask the user if he wants to encrypt or decrypt a text.
        mode = input('Type E for enryption mode or D for decryption mode : ').upper()
        if mode == 'D' or mode == 'E':
            break
        else:
            print('You must enter E or D ! ')
    # This line takes the user's text and converts it to uppercase to search for it in the letter's table.
    while True:
        text = input('Enter your text : ').upper()
        if len(text) != 0:
            break
        else:
            print('You can not leave it blank ! ')
    while True:
        try:
            # This line takes the user's key.
            key = int(input('Enter a key : '))
            if type(key) == int:
                break
        except ValueError:
            print('You must enter an integer number ! ')
    # If the user wants to encrypt a text it will call the caesar() function.
    if mode == 'E':
        caesar()
    # If the user wants to decrypt a text it will make the key value negative so that there's a reverse process
    # and it will call the caesar() function.
    elif mode == 'D':
        key = 0 - key
        caesar()
\$\endgroup\$
5
\$\begingroup\$

First off you if we look at the main of your code, it's mostly all the same pattern.

while True:
    text = input().upper()
    if ...:
        break
    else:
        print()

You should make that a function. And then call it.

def get_input(message, error_message, fn): 
    while True:
        text = input(message).upper()
        if fn(text):
            return text
        else:
            print(error_message)

This severely reduces the amount of code needed. The way you use this new function is:

mode = get_input(
    'Type E for enryption mode or D for decryption mode : ',
    'You must enter E or D ! ',
    lambda mode: mode in 'DE'
)

text = get_input(
    'Enter your text : ',
    'You can not leave it blank ! ',
    lambda text: text
)

If you have not come across lambda before it's a way to make small functions. For example, the following are the same:

my_fn = lambda a: len(a) - 1

def my_fn(a):
    return len(a) - 1

You should also note that I replaced mode == 'D' or mode == 'E' to mode in 'DE'. It's much easier to add to this if you need to. So if you add say 'A' as a function, you can change it to mode in 'ADE'.

Also in Python you should use if text rather than if len(text) != 0. This is as empty objects are False, and others True. E.g.

>>> bool('')
False
>>> bool('a')
True
>>> bool([])
False
>>> bool(['a'])
True

Now onto the bit that is a bit more challenging.
To make a function to check if you can change the input into a number, using int, we can't use lambda. to do this you need to make a 'proper' function.
Alternately you can use str.isdigit. (Thanks @SuperBiasedMan)

def int_able(string):
    try:
        int(string)
        return True
    except ValueError:
        return False

key = int(get_input(
    'Enter a key : ',
    'You must enter an integer number ! ',
    int_able
))

# Or
key = int(get_input(
    'Enter a key : ',
    'You must enter an integer number ! ',
    lambda text: text.strip().isdigit()
))

When calling a function you should always pass it arguments. caesar() is a bad design. For a simple script like this it's alright, however it's not a good habit to get into. Instead, I would recommend that you call it with caesar(text, key).
I would re-write your modes to:

if mode == 'D':
    key = -key

caesar(text, key)

As I recommended you change caesar you will need to change the arguments you pass to it.

You shouldn't add to the letters_table, that should stay the same. If you need to add more letters then you should add them in the definiton.

If you make letters_table a constant, then you can change it to a string.

You should change your loop from for letter in range(len(text)) to for letter in text.

Your algorithm can be changed to one line, and if step < 0 should never happen.

And you should probably remove the if position > 25 it's not a good idea, just add letters to letters_table.

I would discourage print(..., end=''), instead make a string.

And so I would re-write your code to:

LETTERS = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'

def caesar(message, key):
    caesar_text = ''
    for letter in message:
        caesar_text += LETTERS[(LETTERS.index(letter) + key) % 26]

    print(caesar_text)

Note: I don't add to LETTERS if you need to add more then hard-write them in.


To note, I didn't read your comments, there were to many, and not meaning to be rude, but I can read Python, in fact it's more straightforward than English at times.

# If step is lower than 0 then we need to add 26,
# to it to complete the decryption process.
if step < 0:
    step += 26

I know which is easier, and faster, to read by far.

But all in all you have nice code, reduce the amount of comments, and pass argument and you'll do great.

\$\endgroup\$
  • 1
    \$\begingroup\$ LETTERS is already string.ascii_uppercase no need to redefine it. \$\endgroup\$ – 409_Conflict Nov 8 '15 at 5:44
  • 1
    \$\begingroup\$ @MathiasEttinger I didn't include that as OP didn't use import and has beginner. \$\endgroup\$ – Peilonrayz Nov 8 '15 at 11:35
  • \$\begingroup\$ Thank you! You've written a great answer. I'm working on knowing more about concepts I don't know and developing the code more accroding to your suggestions. \$\endgroup\$ – Mahmood Muhammad Nageeb Nov 8 '15 at 14:35
  • \$\begingroup\$ You could make a lambda to test for integers like this: lambda text: text.strip().isdigit() \$\endgroup\$ – SuperBiasedMan Nov 9 '15 at 9:55
  • \$\begingroup\$ @SuperBiasedMan Ah yes, I forgot the function name for isdigit, and chose to be lazy. \$\endgroup\$ – Peilonrayz Nov 9 '15 at 9:58

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.