HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Caesar Encryption-Decryption Tool

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
encryptioncaesartooldecryption

Problem

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 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 decr

Solution

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 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.

Code Snippets

while True:
    text = input().upper()
    if ...:
        break
    else:
        print()
def get_input(message, error_message, fn): 
    while True:
        text = input(message).upper()
        if fn(text):
            return text
        else:
            print(error_message)
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
)
my_fn = lambda a: len(a) - 1

def my_fn(a):
    return len(a) - 1
>>> bool('')
False
>>> bool('a')
True
>>> bool([])
False
>>> bool(['a'])
True

Context

StackExchange Code Review Q#110138, answer score: 6

Revisions (0)

No revisions yet.