feedback-2014-june-27.md

Feedback: July 27, 2014

For reference, here is your original code:

#!/usr/bin/env python

def encrypt (raw_string, shift):
    encrypted_string = ""
    for letter in raw_string:
        zeroed_ord = ord(letter) - 96
        shifted_ord = (zeroed_ord + int(shift)) % 26
        if shifted_ord == 0:
            shifted_ord += 26
        encrypted_string += chr(shifted_ord + 96)
    return encrypted_string
    
def decrypt(encrypted_string, shift):
    return encrypt(encrypted_string, -int(shift))
    
if __name__ == "__main__":
    while True:
        choice = raw_input("This is a Caesar cipher encrypter/decrypter. Enter 'e' to encrypt, 'd' to decrypt, and 'q' to quit: ")
        if choice.lower() == 'e':
            raw_string = raw_input("Enter string to be encrypted. Letters only please: ")
            shift = raw_input("Enter right shift: ")
            print "Your encrypted string is {0}".format(encrypt(raw_string, shift))
        elif choice.lower() == 'd':
            encrypted_string = raw_input("Enter string to be decrypted. Letters only please: ")
            shift = raw_input("Enter right shift: ")
            print "Your decrypted string is {0}".format(decrypt(encrypted_string, shift))
        elif choice.lower() == 'q':
            break
        else:
            print "Invalid choice. Try again... "

Transformation 1: Zero-indexing vs One-indexing

Let's start by taking a look at your encrypt method -- specifically, this line:

zeroed_ord = ord(letter) - 96

I'm not sure if this is intentional, or if this was a bug, but this isn't actually a 'zero'. Running ord('a') actually produces 97, not 96. Therefore, zeroed_ord is actually indexed starting from 1, not from zero.

As a partial consequence of that, you needed to include an if statement in order to properly handle the shifting when you do modulus.

If you adjust that 96 to 97, then you do correctly start at zero, and you can omit the entire if statement.

Here's your adjusted encrypt function:

def encrypt (raw_string, shift):
    encrypted_string = ""
    for letter in raw_string:
        zeroed_ord = ord(letter) - 97
        shifted_ord = (zeroed_ord + int(shift)) % 26
        encrypted_string += chr(shifted_ord + 97)
    return encrypted_string

Transformation 2: Removing inefficiency

In your encrypt function, there are two sources of inefficiency. The first is that you're converting shift into a number with each iteration of the loop. We can fix this by converting

The second is how you're building up the final string. Remember, in Python, strings are immutable -- they can never change. Consequently, when you do something like "abc" + "def", you're actually copying the contents of both strings over and concatenating them.

This turns out to be fairly inefficient, and leading to O(n^2) performance. (Though in practice, Python does apply several optimizations that alleviates this problem).

So, the generally accepted way to concatenate many strings in Python is to first append them all to a list, and then join them at the end.

def encrypt(raw_string, shift):
    encrypted_string = []
    shift = int(shift)
    for letter in raw_string:
        zeroed_ord = ord(letter) - 97
        shifted_ord = (zeroed_ord + shift) % 26
        encrypted_string.append(chr(shifted_ord + 97))
    return ''.join(encrypted_string)

Not only is this more efficient, but it also paves the path to the next transformation.

Transformation 3: Higher-order programming

I think now is a good point to introduce the concept of "higher-order programming". In essence, your encrypt function suffers from the following flaw: it mixes together implementation with intent.

If you think about it, what your encrypt function is doing is taking in a sequence of some items (the original sentence), transforming it, and returning the transformed sequence. In order to make this work, you need to do three separate things: iterate through the string, transform the characters, and build up a new list.

This is a very common pattern in coding. You have an input sequence, you transform, and you return the output. A common habit many beginning programmers have is that they don't recognize this common pattern, and keep re-implementing it (writing for loops, while loops, etc.)

While at first, their code works, it becomes increasingly messier and messier as their loop starts getting more and more complex. For example, what if you need to transform one list into another, but only if some condition is true, then take that output and transform it again, then aggregate it together, etc...

In this case, it may seem difficult to find a way to abstract this out. How do you abstract the idea of a for loop? Of a transformation? Most beginning coders are used to the idea of abstracting data (using lists, dicts, objects), but not so much with abstracting the idea of control flow.

We can take our cues from functional programming to try and find a way to abstract this.

The first thing I'll do is move the code to shift a single letter into its own function:

def shift_letter(letter, shift):
    zeroed_ord = ord(letter) - 97
    shifted_ord = (zeroed_ord + shift) % 26
    return chr(shifted_ord + 97)

def encrypt(raw_string, shift):
    shift = int(shift)
    encrypted_string = []
    for letter in raw_string:
        encrypted_string.append(shift_letter(letter, shift))
    return ''.join(encrypted_string)

Now, if you notice, it's currently taking us exactly three lines to perform the next steps: looping and creating a new list. What we can do is use something called a list comprehension -- a construct Python offers to generalize this pattern. Here's what it looks like:

def shift_letter(letter, shift):
    zeroed_ord = ord(letter) - 97
    shifted_ord = (zeroed_ord + shift) % 26
    return chr(shifted_ord + 97)

def encrypt(raw_string, shift):
    shift = int(shift)
    encrypted_string = [shift_letter(letter, shift) for letter in raw_string]
    return ''.join(encrypted_string)

This fragment of code is exactly identical to the one up above, but makes our intent more clear by not entangling the looping and the transforming. When we use a list comprehension, it implicitly states that the primary goal of the function is to transform some sequence, and that there are no additional side effects.

Granted, this is not the best example of functional-style programming. Pedagogically, this isn't a good example to really introduce the concept because the current example introduces several complications that makes it hard to immediately show all the nifty things you can do.

Regardless, I'll re-introduce the topic in future feedback writeups, so just keep the idea percolating in the back of your mind (and look up list comprehensions for a much better introduction to the subject).

Transformation 4: Additional cleanup

There were a few minor things you could fix.

• You convert the shift to an int in decrypt, yet pass in a string when literally calling encrypt. It'd be better to convert the shift to an integer immediately after recieving it so that we pass in a uniform data type to the encrypt function.
• You call choice.lower() in multiple places -- it'd be better to do it once.

gistfile1.md

So, as a general overview, your code is essentially correct. The only main problem that I see is that you're not very familiar with some common idioms in Python, so my writeup will focus primarily on those.

Transformation 1: Sloppy combination

First, let's format your code. You gave me two files: easy1.py and easy1.5.py. For convenience, and for the sake of learning, I'll combine them into a single file like so:

def ask_user():
    name = raw_input("Name? ")
    age = raw_input("Age? ")
    username = raw_input("Username? ")
     
    f = open("userdata.txt", "w")
    f.write(name + '\n')
    f.write(age + '\n')
    f.write(username + '\n')
     
    print "Your name is %s, you are %s years old, and your username is %s." % (name, age, username)
    
def ask_file():
    data = []
     
    with open("userdata.txt", "r") as f:
            data = [line.strip() for line in f.readlines()]
                   
    print "Your name is %s, you are %s years old, and your username is %s." % (data[0], data[1], data[2])
    
ask_user()

Later on, what we'll do is we'll merge these two so that a single program can both get input from the user and then from the text file.

Transformation 2: Basic boilerplate

So, the first things I'll do is add some 'boilerplate':

#!/usr/bin/env python

def ask_user():
    name = raw_input("Name? ")
    age = raw_input("Age? ")
    username = raw_input("Username? ")
     
    f = open("userdata.txt", "w")
    f.write(name + '\n')
    f.write(age + '\n')
    f.write(username + '\n')
     
    print "Your name is %s, you are %s years old, and your username is %s." % (name, age, username)
    
def ask_file():
    data = []
     
    with open("userdata.txt", "r") as f:
            data = [line.strip() for line in f.readlines()]
                   
    print "Your name is %s, you are %s years old, and your username is %s." % (data[0], data[1], data[2])
    
if __name__ == '__main__':
    ask_user()

The very first line is a kind of comment called a shebang. It's completely ignored in Windows, but in Linux computers, it tells the shell to treat the file as a script which can then be run.

Again, it has no effect on Windows, but it does make life a little easier for Linux users. Typically, this line usually goes at the very top of every Python file you write.

The second change is the if __name__ == '__main__' bit, and is a bit more complicated to explain. In essence, Python will automatically create a variable called __name__. Whenever you are running the Python file directly, Python will set the variable equal to the string '__main__'. Whenever you import your Python file, __name__ will equal something else.

The net effect is that the if statement only runs if you're directly running the file, and will not run when you're only importing the file. This is also a fairly common Python idiom.

Transformation 3: Using with

#!/usr/bin/env python

def ask_user():
    name = raw_input("Name? ")
    age = raw_input("Age? ")
    username = raw_input("Username? ")
     
    with open("userdata.txt", "w") as f:
        f.write(name + '\n')
        f.write(age + '\n')
        f.write(username + '\n')
     
    print "Your name is %s, you are %s years old, and your username is %s." % (name, age, username)
    
def ask_file():     
    with open("userdata.txt", "r") as f:
        data = [line.strip() for line in f.readlines()]
                   
    print "Your name is %s, you are %s years old, and your username is %s." % (data[0], data[1], data[2])
    
if __name__ == '__main__':
    ask_user()

The next step here is to use with. You used it correctly in the bonus portion, but not in the first portion. You typically don't need to manually open a file. If you ever do, don't forget to call f.close() to close the file.

I also corrected your indentation error in ask_file. The with keyword also doesn't introduce a new scope, so there's no need to pre-declare the data variable before-hand.

Transformation 4: Using format, not %

#!/usr/bin/env python 

def ask_user():
    name = raw_input("Name? ")
    age = raw_input("Age? ")
    username = raw_input("Username? ")
     
    with open("userdata.txt", "w") as f:
        f.write(name + '\n')
        f.write(age + '\n')
        f.write(username + '\n')
       
    print_information(name, age, username)
    
def ask_file():     
    with open("userdata.txt", "r") as f:
        data = [line.strip() for line in f.readlines()]

    print_information(*data)
    
def print_information(name, age, username):
    template = 'Your name is {0}, you are {1} years old, and your username is {2}'
    print template.format(name, age, username)
    
if __name__ == '__main__':
    ask_user()

Because in this instance, both our ask_user and ask_file functions both print out the name/age/username, I pulled it out into a separate function. Within that function, I used the .format method, which is now the preferred way of replacing things inside a string instead of using the old % syntax.

There are a lot of nifty things you can do with .format. Although in this case, we're using it to simply interpolate/insert values into a string, you can also use it to do all kinds of formatting. You can skim the examples in the documentation for more information.

The other thing I did was function(*args). The asterisk unpacks the list. The two are exactly identical:

args = ['a', 'b', 'c']

function(*args)
function(args[0], args[1], args[2])

It's a very handy shortcut. You can do something similar with keyword arguments by using two asterisks:

kwargs = {'a': 1, 'b': 2, 'c': 3}

function(**kwargs)
function(a=1, b=2, c=3)

Transformation 5: Condensing reading and writing

#!/usr/bin/env python

def ask_user():
    name = raw_input("Name? ")
    age = raw_input("Age? ")
    username = raw_input("Username? ")
    
    data = [name, age, username] 
    with open("userdata.txt", "w") as f:
        f.write('\n'.join(data))
       
    print_information(name, age, username)
    
def ask_file():     
    with open("userdata.txt", "r") as f:
        data = f.read().split('\n')

    print_information(*data)
    
def print_information(name, age, username):
    template = 'Your name is {0}, you are {1} years old, and your username is {2}'
    print template.format(name, age, username)
    
if __name__ == '__main__':
    ask_user()

Rather then manually writing each item to the file, I decided to instead leverage join and split.

I've always thought that the join method in Python was a little backwards. The form is essentially:

separating_string.join(list_of_strings)

So, for example, ' '.join(['a', 'b', 'c']) will result in 'a b c'.

The split method basically undoes that operation.

Last thoughts

The final thing I'd do is add some code allowing the user to swap between doing either the original task or the bonus. There's numerous ways to do this, but it's not really relevant to the problem at hand, so I'll that out for now.