Is my code too descriptive?

[Cyph0n]

Hello everyone.

Today I decided to write a little Caesar cipher script in Python. After tweaking the code and testing it, I shot up my browser and had a look at other snippets on the web. I was shocked at what I saw.

All of the Python solutions I went over were implemented in ~30 LoC. My implementation is ~160 LoC. I was aware of my "descriptive" coding style i.e. I'm not really good at making my code shorter and more concise, but this was too much. A 130 LoC difference? Goddammit.

I'll have to admit it though: my script can encipher/decipher both words and sentences that can contain a mix of upper and lower-case letters. I'm not sure whether or not the scripts I saw did this as well, but I'm certain that this can be added in less than ~30 LoC.

Well then, here it is. Scary, is it not?

# Sorry for the lack of comments.

from sys import exit
lowercase = ['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']
uppercase = list(''.join(lowercase).upper())

def encrypt_caesar_cipher():
    sentence = raw_input("\nEnter a sentence to encrypt: ")
    shift = input("Enter a key (this is used to decrypt the message): ")
    
    if ' ' in sentence:
        words = sentence.split()
        encrypted_words = []
    else:
        letters = str(sentence)
        encrypted_letters = []
        for letter in letters:
            if letter in lowercase:
                index = lowercase.index(letter)
                if index + shift < 26:
                    index += shift
                    encrypted_letters.append(lowercase[index])
                elif index + shift >= 26:
                    index += shift
                    while index > 25:
                        index -= 26
                    encrypted_letters.append(lowercase[index])
                else:
                    pass
            elif letter in uppercase:
                index = uppercase.index(letter)
                if index + shift < 26:
                    index += shift
                    encrypted_letters.append(uppercase[index])
                elif index + shift >= 26:
                    index += shift
                    while index > 25:
                        index -= 26
                    encrypted_letters.append(uppercase[index])
                else:
                    pass
            else:
                print "\nPlease use upper or lower case letters ONLY next time."
                exit(0)
        print '\n' + ''.join(encrypted_letters)
        exit(0)
        
    
    for word in words:
        letters = str(word)
        encrypted_letters = []
        for letter in letters:
            if letter in lowercase:
                index = lowercase.index(letter)
                if index + shift < 26:
                    index += shift
                    encrypted_letters.append(lowercase[index])
                elif index + shift >= 26:
                    index += shift
                    while index > 25:
                        index -= 26
                    encrypted_letters.append(lowercase[index])
                else:
                    pass
            elif letter in uppercase:
                index = uppercase.index(letter)
                if index + shift < 26:
                    index += shift
                    encrypted_letters.append(uppercase[index])
                elif index + shift >= 26:
                    index += shift
                    while index > 25:
                        index -= 26
                    encrypted_letters.append(uppercase[index])
                else:
                    pass
            else:
                print "\nPlease use upper or lower case letters ONLY next time."
                exit(0)
        result = ''.join(encrypted_letters)
        encrypted_words.append(result)
    
    encrypted_sentence = ' '.join(encrypted_words)
    print "\n" + encrypted_sentence
    
def decrypt_caesar_cipher():
    encrypted = raw_input("\nEnter a sentence to decrypt: ")
    shift = input("Enter the key: ")
    
    if ' ' in encrypted:
        words = encrypted.split()
        decrypted_words = []
    else:
        letters = str(encrypted)
        decrypted_letters = []
        for letter in letters:
            if letter in lowercase:
                index = lowercase.index(letter)
                if index - shift >= 0:
                    index -= shift
                    decrypted_letters.append(lowercase[index])
                elif index - shift < 0:
                    index -= shift
                    while index <= 0:
                        index += 26
                    decrypted_letters.append(lowercase[index])
                else:
                    pass
            elif letter in uppercase:
                index = uppercase.index(letter)
                if index - shift >= 0:
                    index -= shift
                    decrypted_letters.append(uppercase[index])
                elif index - shift < 0:
                    index -= shift
                    while index <= 0:
                        index += 26
                    decrypted_letters.append(uppercase[index])
                else:
                    pass
        print '\n' + ''.join(decrypted_letters)
        exit(0)
    
    for word in words:
        letters = str(word)
        decrypted_letters = []
        for letter in letters:
            if letter in lowercase:
                index = lowercase.index(letter)
                if index - shift >= 0:
                    index -= shift
                    decrypted_letters.append(lowercase[index])
                elif index - shift < 0:
                    index -= shift
                    while index <= 0:
                        index += 26
                    decrypted_letters.append(lowercase[index])
                else:
                    pass
            elif letter in uppercase:
                index = uppercase.index(letter)
                if index - shift >= 0:
                    index -= shift
                    decrypted_letters.append(uppercase[index])
                elif index - shift < 0:
                    index -= shift
                    while index <= 0:
                        index += 26
                    decrypted_letters.append(uppercase[index])
                else:
                    pass
        result = ''.join(decrypted_letters)
        decrypted_words.append(result)
    
    new_sentence = ' '.join(decrypted_words)
    print "\n" + new_sentence

ans = raw_input("\nDecrypt or encrypt?: ")

if ans == 'encrypt':
    encrypt_caesar_cipher()

elif ans == 'decrypt':
    decrypt_caesar_cipher()
    
else:
    print "\nWhat?"

Does this unusual habit of mine make my coding inefficient? Should I change my ways, so I don't become cursed with bad coding habits for the rest of my life? If so, are there any books on coding practices you would recommend?

Please. I'm really worried about where this is might be headed. Any advice or help would be appreciated.

Thanks.

[Enalicho]

No, your code is not "too descriptive". Code can never be too descriptive ;) You've got sensible object names, though I would replace your magic numbers with constants and split your functions into far more smaller functions.

Good code should be self commenting - the code should describe and show what it does without any extra notes. Method names should describe clearly what the method does, and object names should describe what the object is.

If you're worried about code inefficiently, look into the algorithm and try to improve it - but be aware, premature "optimization" often makes code both unreadable and unmaintainable.

For good read on good coding practices, read PEP 8, "Clean Code", and "Code Complete".

[Cyph0n]

Thanks for the feedback and book recommendations. I appreciate it.

[woooee]

You can replace

if ' ' in sentence:
    words = sentence.split()
    encrypted_words = []
else:
#
#etc.  with
words = sentence.split()
for word in words:

If there isn't a space, then "words" will just contain one entry, and the for loop will exit after the first loop. Also

index = lowercase.index(letter)
                if index + shift < 26:
                    index += shift
                    encrypted_letters.append(lowercase[index])
                elif index + shift >= 26:
                    index += shift
                    while index > 25:
                        index -= 26
                    encrypted_letters.append(lowercase[index])
                else:
                    pass
##
##------------- can be replaced with
                index = lowercase.index(letter)
                index += shift
                while index > 25:
                    index -= 26
                encrypted_letters.append(lowercase[index])

as the while loop will never be executed if the index is < 26.

[techie1991]

you might wanna skip things like:

letters = str(encrypted) ## you have taken encrypted as input from raw_input. So it would be a string anyways.

And you can change lowercase to a set rather than a list. Checking for characters in it would become faster.

[techie1991]

you might wanna skip things like:

letters = str(encrypted) ## you have taken encrypted as input from raw_input. So it would be a string anyways.

And you can change lowercase to a set rather than a list. Checking for characters in it would become faster.

[lrh9]

It's not unusual for early implementations of code to be excessive or unrefined.

The important thing is to refactor as necessary.

http://en.wikipedia.org/wiki/Code_refactoring

Your early implementations and final versions will improve over time if you learn more about Python, plan what you want your code to do, decompose data and functions, study other implementations, and ask for feedback.

In terms of code elegance and readability, I don't see the number of lines as a major consideration. In fact, multiple lines are usually preferable to a bunched up one liner.

In terms of comments, there are a few guidelines. You want to avoid useless comments. You don't need to comment every line of code. You don't need to comment obvious code. "#Sets c to a + b." is a useless comment.

How can you tell which code is obvious and which isn't? That is tricky, but I'd ask myself this question: "If I forgot this code, could I look at it and figure out what it does quickly?"

If you think you couldn't, you should add some useful comments explaining what it does.

Useful comments most often give the big picture of what code does. E.g. a useful comment for "c = math.sqrt(a ** 2 + b ** 2)" would be "Calculates the hypotenuse."

Let me offer you my implementation.

##The MIT License
##
##Copyright (c) 2011 Larry Haskins
##
##Permission is hereby granted, free of charge, to any person obtaining a copy
##of this software and associated documentation files (the "Software"), to deal
##in the Software without restriction, including without limitation the rights
##to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
##copies of the Software, and to permit persons to whom the Software is
##furnished to do so, subject to the following conditions:
##
##The above copyright notice and this permission notice shall be included in
##all copies or substantial portions of the Software.
##
##THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
##IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
##FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
##AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
##LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
##OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
##THE SOFTWARE.

#Python 3.1

import string

__all__ = ['rot']

#Case gnostic Caeser cipher tools.

#http://en.wikipedia.org/wiki/Caesar_cipher

UPPERCASE_OFFSET = 65
LOWERCASE_OFFSET = 97

ciphers = dict()

def shift(char, n):
    """Returns Caesar shift n of char."""
    ordinal = ord(char)
    #Caesar shift works only for letters.
    if not char.isalpha():
        raise ValueError(
            "chr({ordinal}) not in alphabet!".format(**locals())
            )
    if char.islower():
        offset = LOWERCASE_OFFSET
    else:
        offset = UPPERCASE_OFFSET
    letter_value = ordinal - offset
    raw_value = letter_value + n
    new_value = raw_value % 26
    new_ordinal = new_value + offset
    new_letter = chr(new_ordinal)
    return new_letter

def cipher(n):
    """Returns Caeser cipher for shift n."""
    if n in ciphers:
        return ciphers[n]
    else:
        cipher = dict()
        for char in string.ascii_letters:
            cipher[char] = shift(char, n)
        global ciphers
        ciphers[n] = cipher
        return cipher

def translate(text, n):
    """Translates text using cipher(n)."""
    cipher_ = cipher(n)
    ciphertext = []
    for char in text:
        try:
            ciphertext.append(cipher_[char])
        except KeyError:
            ciphertext.append(char)
    return ''.join(ciphertext)

def rot(text):
    """Returns ROT13 of text.
    http://en.wikipedia.org/wiki/ROT13"""
    return translate(text, 13)

if __name__ == '__main__':
    #USAGE:
    #Caesar cipher shift 3 encryption and decryption.
    plaintext = 'the quick brown fox jumps over the lazy dog'
    ciphertext = translate(plaintext, 3)
    assert ciphertext == 'wkh txlfn eurzq ira mxpsv ryhu wkh odcb grj'
    assert translate(ciphertext, -3) == 'the quick brown fox jumps over the lazy dog'
    print('How can you tell an extrovert from an introvert at NSA?')
    #ROT13
    print(rot("Va gur ryringbef, gur rkgebireg ybbxf ng gur BGURE thl'f fubrf."))