Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Since I'm a big politics nerd, I wanted to write a little script that would analyze the frequency and content of political fundraising emails. I signed up for the e-mails of 6 campaigns, donated a dollar to each so I'd get hit up for more money often, and let it sit. Unfortunately you won't be able to run the script because it uses some of my personal login details but you could fill your own in if you were really so inclined to give it a run! I was just curious to see if you wonderful people could see anything that could use improvement!

import gmail,pandas as pd,numpy as np,matplotlib.pyplot as plt, plotly.plotly as py, plotly.graph_objs as go,json
from tqdm import tqdm
from collections import Counter
from bs4 import BeautifulSoup
from textblob import TextBlob
from textblob.en.sentiments import NaiveBayesAnalyzer
import sys,indicoio
indicoio.config.api_key = '#'
reload(sys)
sys.setdefaultencoding('utf8')

r=open('emails.csv','w')

py.sign_in('#','#')

cruzbin=[];trumpbin=[];clintbin=[];rubiobin=[];christiebin=[];jebbin=[]
politicians = ['tedcruz.org','donaldjtrump.com','donaldtrump.com','hillaryclinton.com','marcorubio.com','jeb2016.com','chrischristie.com']

def start():
    g = gmail.login('#','#')
    return g

def sorter(g):
    for _ in tqdm(g.inbox().mail(sender=politicians[0],prefetch=True)):
        cruzbin.append(_)
    for _ in g.inbox().mail(sender=politicians[1],prefetch=True):
        trumpbin.append(_)
    for _ in g.inbox().mail(sender=politicians[2],prefetch=True):
        trumpbin.append(_)
    for _ in g.inbox().mail(sender=politicians[3],prefetch=True):
        clintbin.append(_)
    for _ in g.inbox().mail(sender=politicians[4],prefetch=True):
        rubiobin.append(_)
    for _ in g.inbox().mail(sender=politicians[5],prefetch=True):
        jebbin.append(_)
    for _ in g.inbox().mail(sender=politicians[6],prefetch=True):
        christiebin.append(_)
    bins = [cruzbin,trumpbin,clintbin,rubiobin,jebbin,christiebin]
    return bins

def counter(bins):
    print 'Emails from Ted Cruz:',len(cruzbin)
    print 'Emails from Donald Trump:',len(trumpbin)
    print 'Emails from Hillary Clinton:',len(clintbin)
    print 'Emails from Marco Rubio:',len(rubiobin)
    print 'Emails from Chris Christie:',len(christiebin)
    print 'Emails from Jeb Bush:',len(jebbin)

def q():
    ans = input('Whose e-mails do you want to analyze?\n')
    return ans

def analyzer(bins):
    tc = pd.DataFrame(cruzbin)
    dt = pd.DataFrame(trumpbin)
    hc = pd.DataFrame(clintbin)
    mr = pd.DataFrame(rubiobin)
    jb = pd.DataFrame(jebbin)
    cc = pd.DataFrame(christiebin)
    trace0 = go.Bar(
        x=['Ted Cruz','Donald Trump','Hillary Clinton','Marco Rubio','Jeb Bush','Chris Christie'],
        y=[len(cruzbin),len(trumpbin),len(clintbin),len(rubiobin),len(jebbin),len(christiebin)],
        marker=dict(
            color=['rgb(204,204,204)','rgb(204,204,204)','rgb(204,204,204)','rgb(204,204,204)','rgb(204,204,204)','rgb(204,204,204)']),
    )
    data = [trace0]
    layout = go.Layout(
        title='Frequency of Fundraising E-Mails',
        )
    fig = go.Figure(data=data,layout=layout)
    plot_url = py.plot(fig,filename='emailfreq')    
    return

def bayes(b,name):
    overallpos = []
    overallneg = []
    i = 0
    for email in tqdm(b):
        try:
            i = i+1
            text = email.fetch()
            text = str(text)
            soup = BeautifulSoup(text,'lxml')
            text = soup.get_text()
            text = text.strip()
            blob = TextBlob(text,analyzer=NaiveBayesAnalyzer())
            senti = indicoio.sentiment_hq(text)
            keywords = indicoio.keywords(text)
            #neg = blob.sentiment.p_neg
            b = blob.np_counts
            c = Counter(b).most_common(10)
            #print c
            #overallpos.append(pos)
            r.write(name)
            r.write(',')
            r.write(str(i))
            r.write(',')
            r.write('null')
            r.write(',')
            r.write(str(senti))
            r.write(',')
            r.write(str(keywords))
            r.write(',')
            r.write(str(c))
            r.write('\n')
            #overallneg.append(neg)
        except UnicodeDecodeError:
            print('Moving on..')
        finally:
            pp = (sum(overallpos))
            #nn = (sum(overallneg))
        pp = sum(overallpos)
        #nn = sum(overallneg)
    #print name,'\'s positive ranking: %s' % (pp / len(overallpos))
    return


def sentiment(bins):
    bayes(cruzbin,'Ted Cruz')
    bayes(trumpbin,'Donald Trump')
    bayes(clintbin,'Hillary Clinton')
    bayes(jebbin,'Jeb Bush')
    bayes(rubiobin,'Marco Rubio')
    bayes(christiebin,'Chris Christie')

def shebang():
    s = start()
    counter(sorter(s))
    analyzer(s)
    sentiment(s)
    return

def starter():
    print('Counter will return the number of e-mails each candidate has sent.  Sentiment will perform sentiment analysis on all of the candidate\'s e-mails and write the results to csv.\
            Analyzer will create a visualization of the number of e-mails each candidate has sent.  Shebang will do all of the above!')
    ans = input('What should I do? \n')
    return ans

def logo():
    print """
                                oooo   o8o               .                                            
                                `888   `"'             .o8                                            
            oo.ooooo.  .ooooo.   888  oooo   .oooo.o .o888oo  .ooooo.   .ooooo.   .oooooooo  .ooooo.  
            888' `88b d88' `88b  888  `888  d88(  "8   888   d88' `88b d88' `88b 888' `88b  d88' `88b 
            888   888 888   888  888   888  `"Y88b.    888   888   888 888   888 888   888  888ooo888 
            888   888 888   888  888   888  o.  )88b   888 . 888   888 888   888 `88bod8P'  888    .o 
            888bod8P' `Y8bod8P' o888o o888o 8""888P'   "888" `Y8bod8P' `Y8bod8P' `8oooooo.  `Y8bod8P' 
            888                                                                  d"     YD            
            o888o                                                                 "Y88888P'            

    How many times do politicians hit up regular people for money?  How positive or negative are their e-mails?\n 
    What do they usually talk about? This program crunches the numbers so you don't have to!   

        Logging in...\n"""
    return

logo()
while True:
    s = start()
    starter()
    try:
        if ans.lower()=='counter':
            counter(sorter(s))
        elif ans.lower()=='sentiment':
            sentiment(s)
        elif ans.lower()=='analyzer':
            analyzer(s)
        elif ans.lower()=='shebang':
            shebang()
    except:
        print('Oops! Try again!')
#analyzer(s)
#sentiment(s)

Example output:

https://plot.ly/~ntucker1/96/frequency-of-fundraising-e-mails/
Emails from Ted Cruz: 17
Emails from Donald Trump: 5
Emails from Hillary Clinton: 25
Emails from Marco Rubio: 35
Emails from Chris Christie: 20
Emails from Jeb Bush: 17

For those that are interested, I'm hosting this app here. It's a work in progress right now as I'm trying to set up the database.

share|improve this question
2  
This looks quite interesting. Could you also provide some example output that your program generates? It would make your question even more interesting. – Simon Forsberg Jan 29 at 9:48
    
Certainly! Added some example output :-) – n1c9 Jan 29 at 18:25
up vote 11 down vote accepted

Let's start with the obvious: this code doesn't run. You're missing ans = starter() so that further (el)if ans.lower() == ... doesn't miserably fail with a NameError.

Likely, you define q() but never use it.

And you also appears to have other useless stuff floating around: why use both textblob and indicoio to perform sentiment analysis? You also seem to never use the blob produced by the textblob analyzer. Same for overallpos or overallneg: no content whatsoever added to them. And a lot of comments that are just old test code being removed…

Improve data structure

Having several variables to hold data for a single logical entity is a mess. Especially if you have several of those entities. The first step is to make a class out of this logical entity so a single variable hold everything you would want to know about it. Second, use a list of these entities and iterate over this list instead of manually writing each element each time: you will avoid inconsistencies like calling tqdm only for retrieving the mails associated to tedcruz.org.

In python, if you only want to store attributes and not build a full blown class, you can use a namedtuple:

from collections import namedtuple
Politician = namedtuple('Politician', 'name emails bin')

politics = [
    Politician('Ted Cruz', ['tedcruz.org'], []),
    Politician('Donald Trump', ['donaldtrump.com', 'donaldjtrump.com'], []),
    Politician('Hillary Clinton', ['hillaryclinton.com'], []),
    Politician('Marco Rubio', ['marcorubio.com'], []),
    Politician('Chris Christie', ['chrischristie.com'], []),
    Politician('Jeb Bush', ['jeb2016.com'], []),
]

You will most likely avoid ordering them differently in various parts of your code, leading to confusions.

Improve processing

Having your politicians in a list will let you focus on the task you want to perform on each one of them, instead of repeatedly copy/pasting your code and possibly introducing bugs.

Each time you would have 6 cases, one for each candidate, use a for loop:

def sorter(g):
    for politician in politics:
        for address in politician.emails:
            for mail in tqdm(g.inbox().mail(sender=address, prefetch=True)):
                politician.bin.append(mail)

Even better, use a list-comprehension here:

def sorter(g, politics):
    for politician in politics:
         politician.bin[:] = [mail
             for address in politician.emails
             for mail in g.inbox().mail(sender=address, prefetch=True)
         ]

Same for printing:

def counter(politics):
    for politician in politics:
        print 'Emails from {}:'.format(politician.name), len(politician.bin)

And for statistics:

def sentiments(politics):
    for politician in politics:
        bayes(politician.bin, politician.name)

It's even more for the analyzer as you heavily rely on knowing you always have 6 politicians. Using namedtuples, you can turn them back into regular tuples and use regular sequence manipulation on them:

def analyzer(politics):
    names, emails, bins = zip(*politics)
    trace0 = go.Bar(
        x=names,
        y=[len(bin) for bin in bins],
        marker=dict(color=['rgb(204,204,204)'] * len(politics)),
    )
    layout = go.Layout(
        title='Frequency of Fundraising E-Mails',
    )
    fig = go.Figure(data=[trace0], layout=layout)
    plot_url = py.plot(fig, filename='emailfreq')

For this one, please, avoid the bare return at the end, it's just noise. And since you are not using any pandas feature, why bother converting your bins into dataframes at all?

Did you notice how I passed politics as parameter to all these function calls? You should avoid relying on global variables and use parameters instead: it lets you reuse and test parts of your code more easily.

Improve file handling

You open a file at the beginning of your program without:

  1. closing it;
  2. knowing if you will need to write into it.

Since you do need it only for sentiments, you should handle it there. The proper way to do that in Python is using the with statement so that you file gets closed anyway at the end of the statement. Whether everything went right or wrong:

def sentiments(politics, filename):
    with open(filename, 'w') as output:
        for politician in politics:
            bayes(politician.bin, politician.name, output)

You will need to modify bayes as well to accept output as the third parameter and not rely on the r global variable.

Improve the global flow

You can't do sentiments analysis with empty bins. You can't plot either. So you should really perform counter(sorter(s)) anyway and then ask the user.

Also take a habit of wrapping your top-level code into if __name__ == '__main__': it's cleaner and you avoid running code when importing your module for testing:

if __name__ == '__main__':
    indicoio.config.api_key = '#'
    py.sign_in('#','#')

    logo()
    s = start()
    Politician = namedtuple('Politician', 'name emails bin')

    politics = [
        Politician('Ted Cruz', ['tedcruz.org'], []),
        Politician('Donald Trump', ['donaldtrump.com', 'donaldjtrump.com'], []),
        Politician('Hillary Clinton', ['hillaryclinton.com'], []),
        Politician('Marco Rubio', ['marcorubio.com'], []),
        Politician('Chris Christie', ['chrischristie.com'], []),
        Politician('Jeb Bush', ['jeb2016.com'], []),
    ]
    sorter(g, politics)
    counter(politics)
    while True:
        action = starter().lower()
        if action in ('sentiment', 'shebang'):
            sentiments(politics, 'emails.csv')
        if action in ('analyzer', 'shebang'):
            analyzer(politics)

Using this, you don't over-define actions (with your shebang function). Also, if something goes wrong, the loop is stopped with a proper error message and you can figure out what went wrong (instead of just knowing that something when wrong with absolutely no way of knowing what).

And, even though I removed the except clause, never use bare excepts: they can swallow deep problems like MemoryError or SystemError. Even the KeyboardInterupt was swallowed, leaving the user literally no way to exit the while loop.

Cleanup your imports (and various other bits)

On top of having unnecessary ones, you should really put one import statement per line and not group them as your first line.

PEP8 also recommends putting standard library imports at the top.

And remove that setdefaultencoding call in favor of a proper encoding declaration.

share|improve this answer
    
thank you SO much for the detailed review!! – n1c9 Jan 28 at 22:23
    
Nitpick: I would like to point out that bare except explicitly does not catch EOFError, SyntaxError or KeyboardInterrupt (at least in 3.x) on its own. – cat Jan 29 at 1:04
    
These are backwards: Politician('Chris Christie', 'jeb2016.com', []), Politician('Jeb Bush', 'chrischristie.com', []), – Quill Jan 29 at 1:18
    
@cat Just tested and KeyboardInterrupt are caught by bare except. Both in Python 2 and Python 3. – Mathias Ettinger Jan 29 at 7:51
1  
@Quill Oh, yeah. Didn't even paid attention to what I was doing. I just copy/pasted stuff in the order it appeared. One more reason to not store related stuff into different variables scattered around the code. – Mathias Ettinger Jan 29 at 7:53

Imagine how much of a pain it would be to add another politician to this list. In addition to the Politician class suggested in another answer, I would suggest creating a list of Politicians and whenever you want to do something to one of them, iterate over this list. This may require adding a method to your Politician class. Then, adding another candidate to the pool (Bloomberg) will only require appending to your list of Politicians.

share|improve this answer

Setting aside PEP 8 (official style guide) issues I would make the following change:

Rather than keeping your politician bins, names and email addresses in separate data structures (the only way they are tied right now is by the array index number) - I would create a Politician class. Something like:

class Politician(object):
    def __init__(self, name, email):
        self.name = name
        self.email = email
        self.bin = []

    // Other methods as necessary
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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