10
\$\begingroup\$

Update

I've posted a follow up question to this code. Find it here:

Not a very interesting implementation of a Simple Substitution Cipher in Python.


I implemented a second encryption algorithm: the well known . This turned out to be longer than I expected. The "main" code is very concise, but the interface that followed seems a bit un-pythonic. Here it is:

caesar.py

"""
This python file contains the "shift()" method to encrypt as well as decrypt
strings.

Help on command line arguments:
1. Use without any of those to enter an interactive mode.
2. Pass only the plaintext to encrypt using default settings (shift of 97
with alphabet of all printable ascii characters).
3. Pass the shift amount (positive to encrypt, negative to decrypt) and one
of "lower", "upper" or "all" to choose the alphabet.
"""
import string
import sys


def shift(message, amount=97, alphabet=string.printable):
    """
    This method takes a string and shifts it by the specified amount. A
    positive amount shifts the characters up, while a negative amount shifts
    the characters down.

    :param message:     The message to be encrypted.
    :param amount:      The amount to shift the message by. (Default is 97)
    :param alphabet:    The alphabet for determining the result of a shift.
                        (Default is all printable characters)
    :return:
    """
    # This handles all the "case issues"
    if alphabet == string.ascii_lowercase:
        message = str.lower(message)
    elif alphabet == string.ascii_uppercase:
        message = str.upper(message)
    shifted_alphabet = alphabet[amount:] + alphabet[:amount]
    table = str.maketrans(alphabet, shifted_alphabet)
    return message.translate(table)


if __name__ == '__main__':
    argument_count = len(sys.argv)
    text = ''
    offset = 97
    if argument_count == 1:
        # The script is being used independently, so display menus and stuff
        print('\n\n'
              '+--------------------------------------+\n'
              '|------------- caesar.py --------------|\n'
              '+--------------------------------------+\n\n')
        text = input('Please input the text to be encrypted : ')
        offset = int(input('Please enter the shift amount (negative to '
                           'decrypt) : '))
        print('The alphabet sets :\n'
              '1. Lowercase ascii\n'
              '2. Uppercase ascii\n')
        choice = input(
            'Please enter the choice number, or anything else to choose '
            'default alphabet (all printable ascii characters): ')
        print('The encrypted text is : ')
        if str.isnumeric(choice):
            number = int(choice)
            if number == 1:
                print(shift(text, offset, string.ascii_lowercase))
            elif number == 2:
                print(shift(text, offset, string.ascii_uppercase))
        else:
            print(shift(text, offset))
    elif argument_count == 2:
        # This and the next mode must be as silent as possible.
        text = sys.argv[1]
        print(shift(text))
    elif argument_count == 4:
        text = sys.argv[1]
        offset = int(sys.argv[2])
        universe = sys.argv[3]
        if universe == 'lower':
            print(shift(text, offset, string.ascii_lowercase))
        elif universe == 'upper':
            print(shift(text, offset, string.ascii_uppercase))
        elif universe == 'all':
            print(shift(text, offset, string.printable))
        else:
            print('Invalid choice of alphabet : {}'.format(universe))
            exit(1)
    else:
        print('Incorrect argument count : {}'.format(argument_count))
        print('Usage:')
        print('$ python3 caesar.py')
        print('\nOR\n')
        print('$ python3 caesar.py [text]')
        print('\nOR\n')
        print('$ python3 caesar.py [text] [shift] [alphabet]')
        exit(2)

Here are some sample runs:

$ python3 caesar.py


+--------------------------------------+
|------------- caesar.py --------------|
+--------------------------------------+


Please input the text to be encrypted : I came, I saw, I conquered.
Please enter the shift amount (negative to decrypt) : 10
The alphabet sets :
1. Lowercase ascii
2. Uppercase ascii

Please enter the choice number, or anything else to choose default alphabet (all printable ascii characters): 
The encrypted text is : 
S4mkwo@4S4CkG@4S4myxAEoBon\

$ python3 caesar.py


+--------------------------------------+
|------------- caesar.py --------------|
+--------------------------------------+


Please input the text to be encrypted : S4mkwo@4S4CkG@4S4myxAEoBon\
Please enter the shift amount (negative to decrypt) : -10
The alphabet sets :
1. Lowercase ascii
2. Uppercase ascii

Please enter the choice number, or anything else to choose default alphabet (all printable ascii characters): 
The encrypted text is : 
I came, I saw, I conquered.

$ python3 caesar.py "Viva La CLI!"
Sfs7|I7|zIFX

$ python3 caesar.py "Sfs7|I7|zIFX" -97 all
Viva La CLI!

I am not quite satisfied with the latter part of the program. Perhaps there's a more Pythonic way to do this? I welcome comments on all parts of the code.

\$\endgroup\$
0

2 Answers 2

8
\$\begingroup\$

I started by running your program, and poking it a bit to see what happens if I misbehave. This is a good way to catch errors and edge cases. Here’s what I found:

  • ValueError if I give bad data for “Please enter the shift amount”.

    Examples:

    Please enter the shift amount (negative to decrypt) :
    Traceback (most recent call last):
      File "caesarcr.py", line 49, in <module>
        offset = int(input('Please enter the shift amount (negative to '
    ValueError: invalid literal for int() with base 10: ''
    

    Please enter the shift amount (negative to decrypt) : notanumber
    Traceback (most recent call last):
      File "caesarcr.py", line 49, in <module>
        offset = int(input('Please enter the shift amount (negative to '
    ValueError: invalid literal for int() with base 10: 'notanumber'
    

    You should catch the ValueError, and either:

    • Default the shift if I enter nonsense, or
    • Keep prompting me until I give valid input, or
    • End the program with an error message

    But a traceback is rarely a good form of error for an end-user.

  • No output if I enter a number that isn’t 1 or 2 at “choice number”.

    I can repeat this with any choice of number; I just don’t get any output:

    Please input the text to be encrypted : hello world
    Please enter the shift amount (negative to decrypt) : 3
    The alphabet sets :
    1. Lowercase ascii
    2. Uppercase ascii
    
    Please enter the choice number, or anything else to choose default alphabet
    (all printable ascii characters): 3
    The encrypted text is :
    

    The problem is these branches here (comments mine):

    if str.isnumeric(choice):
        number = int(choice)
        if number == 1:
            # do stuff with lowercase ASCII
        elif number == 2:
            # do stuff with uppercase ASCII
      # else:
      #     What about input that falls down here?
    else:
        # do stuff with mixed case
    

    You need to add that missing else: branch.

  • I get unchanged output if I use offset ≥ 100.

    Some examples:

    $ python3 caesarcr.py "hello world" 99 all
    gdkkn~vnqkc
    
    $ python3 caesarcr.py "hello world" 100 all
    hello world
    
    $ python3 caesarcr.py "hello world" 101 all
    hello world
    

    I suspect this is related to the fact that the length of string.printable is 100, and you’re failing to wrap around correctly. Perhaps it’s okay that shifting by 100 gives the same output, but after that it should reset – so offset by 101 is the same as offset by 1, etc.


Playtime over, time to dive into the code. A few items of note:

  • Inside shift(), your calls to str.upper() and str.lower() can be rewritten as:

    message = message.lower()
    message = message.upper()
    

    That’s the more approach I see more often.

  • I think this line is the source of your wrap-around woes:

    shifted_alphabet = alphabet[amount:] + alphabet[:amount]
    

    You should probably set amount to be amount % len(alphabet) before this line, then I think you’d be okay.

  • I’m not sure why 97 is the default shift length, or if it’s even appropriate to have a default shift. At the very least, there should be a comment to explain this choice.

  • It’s not clear why I can’t supply two arguments, say:

    $ python3 caesarcr.py 'hello world' 3
    

    and have the script automatically use the full set of ASCII characters for me.

  • To break up the main() block, I’d wrap the interactive and command-line help in two different functions. Then you’d have:

    if __name__ == '__main__':
        if len(sys.argv) == 1:
            interactive_encrypt()
        elif len(sys.argv) <= 4:
            command_line_encrypt()
        else:
            print_usage_message()
    

    Within the command-line parser, you can tidy it up slightly by combining cases. For example:

    text = sys.argv[1]
    
    if len(sys.argv) >= 2:
        offset = int(sys.argv[2])
    else:
        offset = 97
    
    if len(sys.argv) >= 3:
        mode = sys.argv[3]
    else:
        mode = 'universe'
    

    rather than, for example, getting text = sys.argv[1] for each length individually.

\$\endgroup\$
1
  • \$\begingroup\$ I guess that's what happens when I chew off more than I can swallow. Anyway... you asked abut the 97. I just realized that I wanted ROT-47 to be the default (half of the printable, non-whitespace characters, totalling to a 94). But I forgot all those and (incorrectly) remembered 97. It is basically the ASCII equivalent of the ROT-13. \$\endgroup\$ Commented Nov 28, 2015 at 15:24
8
\$\begingroup\$

You've got lots of stuff under if __name__ == '__main__'. This is almost always bad; using a main function normally works out nicer. You use exit rather than sys.exit, FWIW, which is not great, and the call should go outside of main().

You do

message = str.lower(message)

I would recommend casefold in this instance, as it will lowercase more letters than otherwise possible. For example, run 'ß'.casefold() and 'ß'.lower(). Alternatively, consider writing this so that only letters which can get translated do.

But, I'll be honest, I don't like this behaviour. Why should passing in

'abcdefghijklmnopqrstuvwxyz'

cause the message to be lowecased automatically but passing in the Swedish alphabet,

'abcdefghijklmnopqrstuvwxyzååö'

not do that at all? This seems ripe for errors.

Further, this behaviour isn't mentioned in the docstring! That'll cause someone problems some months down the line when they use a slightly different set of values and their text comes out uppercased!

A minor, but nagging, point here is your use of class.method(object) over object.method(). This is poor form, and you probably shouldn't be doing it.

There's also the default for amount of 97. That doesn't make sense! What if I pass in alphabet="abc"? Why would a shift of 97 make sense? I suggest not giving a default here, and probably no default alphabet either.

You default initialize

text = ''
offset = 97

but never use 'em. That's a good way to hide bugs - don't do it.

I would deduplicate the actual shifting and outputting by acquiring the arguments separately. First check

if argument_count not in (1, 2, 4):
    print('Incorrect argument count : {}'.format(argument_count))
    print('Usage:')
    print('$ python3 caesar.py')
    print('\nOR\n')
    print('$ python3 caesar.py [text]')
    print('\nOR\n')
    print('$ python3 caesar.py [text] [shift] [alphabet]')
    return 2

Then at the end put

if universe == 'lower':
    print(shift(text.casefold(), offset, string.ascii_lowercase))
elif universe == 'upper':
    print(shift(text.upper(), offset, string.ascii_uppercase))
elif universe == 'all':
    print(shift(text, offset, string.printable))
else:
    raise NotImplementedError

Then you can patch the middle with something like:

if argument_count == 1:
    # The script is being used independently, so display menus and stuff
    print('\n\n'
          '+--------------------------------------+\n'
          '|------------- caesar.py --------------|\n'
          '+--------------------------------------+\n\n')
    text = input('Please input the text to be encrypted : ')
    offset = int(input('Please enter the shift amount (negative to '
                       'decrypt) : '))
    print('The alphabet sets :\n'
          '1. Lowercase ascii\n'
          '2. Uppercase ascii\n')
    choice = input(
        'Please enter the choice number, or anything else to choose '
        'default alphabet (all printable ascii characters): ')
    if choice.isnumeric():
        number = int(choice)
        if number == 1:
            universe = 'lower'
        elif number == 2:
            universe = 'upper'
    else:
        universe = 'all'
    print('The encrypted text is : ')

elif argument_count == 2:
    # This and the next mode must be as silent as possible.
    text = sys.argv[1]
    offset = 97
    universe = 'all'

elif argument_count == 4:
    text = sys.argv[1]
    offset = int(sys.argv[2])
    universe = sys.argv[3]
    if universe not in ('lower', 'upper', 'all'):
        print('Invalid choice of alphabet : {}'.format(universe))
        return 1

Note this part:

if choice.isnumeric():
    number = int(choice)
    if number == 1:
        universe = 'lower'
    elif number == 2:
        universe = 'upper'
else:
    universe = 'all'

This is missing the case for choice.isnumeric() and int(choice) not in (1, 2). Instead, why not just do

universe = {'1': 'lower', '2': 'upper'}.get(choice, 'all')

Frankly, this is horrid:

offset = int(input('Please enter the shift amount (negative to '
                   'decrypt) : '))

The 80-character limit is a suggestion, not a rule. This looks way nicer:

offset = int(input('Please enter the shift amount (negative to decrypt) : '))

so you should prefer it.

Then, since you're doing command-line parsing I'd move to docopt and hoist out dynamic aquiring of arguments:

"""
This python file contains the "shift()" method to encrypt as well as decrypt
strings.

Help on command line arguments:
1. Use without any of those to enter an interactive mode.
2. Pass only the plaintext to encrypt using default settings (shift of 97
with alphabet of all printable ascii characters).
3. Pass the shift amount (positive to encrypt, negative to decrypt) and one
of "lower", "upper" or "all" to choose the alphabet.

Usage:
    caesar.py
    caesar.py <text>
    caesar.py <text> <shift> (lower | upper | all)
"""

import string
import sys

import docopt


def shift(message, amount, alphabet):
    """
    This method takes a string and shifts it by the specified amount. A
    positive amount shifts the characters up, while a negative amount shifts
    the characters down.

    :param message:     The message to be encrypted.
    :param amount:      The amount to shift the message by.
    :param alphabet:    The alphabet for determining the result of a shift.
    :return:
    """
    shifted_alphabet = alphabet[amount:] + alphabet[:amount]
    table = str.maketrans(alphabet, shifted_alphabet)
    return message.translate(table)


def get_args_dynamically():
    # The script is being used independently, so display menus and stuff
    print('\n\n'
          '+--------------------------------------+\n'
          '|------------- caesar.py --------------|\n'
          '+--------------------------------------+\n\n')
    text = input('Please input the text to be encrypted : ')
    offset = int(input('Please enter the shift amount (negative to decrypt) : '))
    print('The alphabet sets :\n'
          '1. Lowercase ascii\n'
          '2. Uppercase ascii\n')
    choice = input(
        'Please enter the choice number, or anything else to choose '
        'default alphabet (all printable ascii characters): ')
    universe = {'1': 'lower', '2': 'upper'}.get(choice, 'all')
    print('The encrypted text is : ')

    return text, offset, universe


def main(args):
    text = args['<text>']

    if text is None:
        text, offset, universe = get_args_dynamically()
    else:
        offset = args['<offset>']
        if offset is None:
            offset = 97

        universe = (
            'lower' if args['lower'] else
            'upper' if args['upper'] else
            'all'
        )

    if universe == 'lower':
        print(shift(text.casefold(), offset, string.ascii_lowercase))
    elif universe == 'upper':
        print(shift(text.upper(), offset, string.ascii_uppercase))
    elif universe == 'all':
        print(shift(text, offset, string.printable))
    else:
        raise NotImplementedError


if __name__ == '__main__':
    sys.exit(main(docopt.docopt(__doc__)))

I would mention the other things alexwlchan mentions but, well, they've already been mentioned.

\$\endgroup\$
1
  • \$\begingroup\$ I wondered if learning Python would be worth the effort. Now I'm just mad at myself for not learning it earlier. \$\endgroup\$ Commented Nov 28, 2015 at 15:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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