7
\$\begingroup\$

I was wondering if someone could tell me things I could improve in this code. This is one of my first Python projects. This program gets the script of a movie (in this case Interstellar) and then finds the occurrences of all words and prints out the 20 most common words in the script.

from bs4 import BeautifulSoup
import requests
from collections import Counter 
import operator
import re

class MovieScript(object):
    def __init__(self, movie):
        self.movie = movie
        self.url = "http://www.springfieldspringfield.co.uk/movie_script.php?movie={0}".format(movie.replace(" ", "-"))

    def get_movie(self):
        return self.movie

    def get_text(self):
        r = requests.get(self.url)
        soup = BeautifulSoup(r.text)
        return soup

    def parse_text(self):
        text = self.get_text().findAll('div', {"class":"scrolling-script-container"})[0].text.lower()
        text = re.sub(r'\W+', ' ', text) #remove puncation from the text
        return text.split()

    def find_all_occurences(self):
        return Counter(self.parse_text())

    def get_all_occurences(self):
        sorted_occurences = sorted(self.find_all_occurences().items(), key=operator.itemgetter(1))
        return sorted_occurences

def Main():
    db = MovieScript("interstellar")
    print "The Movie: " + db.get_movie()
    all_occurences = db.get_all_occurences()
    for i in range(-1, -21, -1):
        print str(-i) + ". " + str(all_occurences[i][0]) + " -> " + str(all_occurences[i][1])


if __name__ == "__main__":
    Main()
\$\endgroup\$
1
  • \$\begingroup\$ I would also upgrade to Python 3. It's the present and future of the language. If you're starting a project today, go for Python 3, unless you really depend on a library (among a few) which is still Python 2. \$\endgroup\$ – Pablo Jun 16 '15 at 0:59
8
\$\begingroup\$

A few things I can think of are:

  • parse_text can be marked as a private method. def _parse_text(self):. I understand this won't actually be hidden, but its a convention followed in python to treat methods starting with an underscore as private.
  • In MovieScript __init__ method maybe have the url as a kwarg (defaulting to http://www.springfieldspringfield.co.uk, but can be overwritten and point to another site in case you ever have to scrape another site)
  • In def get_text: You can cache the result from the request.get you're running and check the cache before making the request
  • Finally consider moving the logic of calculating the top keywords inside the MovieScript class itself (call it something like def get_common_words(self, count=20):) and allow the count to change in case you ever want more than the default.

I'm sure there are more and people will have there own preferences, but this is just what I could suggest in a quick glimpse :)

\$\endgroup\$
1
  • \$\begingroup\$ The parsing of the page (div.scrolling-script-container) is dependent on the website's formatting. If you want this to handle other movie script websites, you'll also need to know how those websites are formatted. \$\endgroup\$ – IceArdor Dec 17 '14 at 21:46
4
\$\begingroup\$

Iterations, the right way

Whenever you are using indices to iterate over an array (or any kind of iterable objects), you are probably doing something wrong. First, it tedious to write. Then it's awkward to check if you don't screw up with off-by-one error. If you really really want indices, just use enumerate, it usually solves your problem.

Now, back to your code, you sort a list and then try to iterate on it starting at the end. Wouldn't it be easier to just sort in reverse order and then iterate normally ?

def get_all_occurences(self):
    sorted_occurences = sorted(self.find_all_occurences().items(), key=operator.itemgetter(1), reverse=True)
    return sorted_occurences

[...]

for i in range(20):
    print str(i+1) + ". " + str(all_occurences[i][0]) + " -> " + str(all_occurences[i][2])

It already looks a lot nicer but we are still playing with indices.

Introducing our common friend enumerate paired with some list slicing :

   for i, occ in enumerate(all_occurences[:20]):
        print str(i+1) + ". " + str(occ[0]) + " -> " + str(occ[1])

Now, we can remove the ugly i+1 by starting counting at 1.

   for i, occ in enumerate(all_occurences[:20], start=1):
        print str(i) + ". " + str(occ[0]) + " -> " + str(occ[1])

Tuple unpacking

It does look better now but still, something makes me a bit sad inside. We still have indices. It corresponds to the fact that we have 2 pieces of information in occ: the word and the number. You can use tuple unpacking to extract the data into 2 variables with meaningful names :

    word, number = occ
    print str(i) + ". " + str(word) + " -> " + str(number)

You can also unpack as you iterate :

   for i, (word, number) in enumerate(all_occurences[:20], start=1):
        print str(i) + ". " + str(word) + " -> " + str(number)

It doesn't get any more concise that this.

String formatting

Instead of using string concatenation, you can use string formatting :

    print "%d. %s -> %d" % (i, word, number)

Counter is even better than you thought

Counter is a pretty cool tool, it even has a most_common method doing exactly what you want.

Your code becomes :

    def get_counter(self):
        return Counter(self.parse_text())

def Main():
    db = MovieScript("interstellar")
    print "The Movie: " + db.get_movie()
    for i, (word, number) in enumerate(db.get_counter().most_common(20), start=1):
        print "%d. %s -> %d" % (i, word, number)

You don't need a class

I wanted this to be the first point but it was easier to find details first. So let's make the point clear : you don't need a class.

Having a structure containing the name and the url adds complexity for no good reason. Also, it makes code hard to reuse if was has to create and instance.

Plain old functions passing data around are more than enough for you :

from bs4 import BeautifulSoup
import requests
from collections import Counter
import operator
import re


def get_url_for_movie(movie_name):
    return "http://www.springfieldspringfield.co.uk/movie_script.php?movie={0}".format(movie_name.replace(" ", "-"))

def get_soup_at_url(url):
    r = requests.get(url)
    return BeautifulSoup(r.text)

def extract_script_from_soup(soup):
    return soup.findAll('div', {"class":"scrolling-script-container"})[0].text

def split_text(text):
    return re.sub(r'\W+', ' ', text.lower()).split() #remove puncation from the text

def pretty_print_list_of_n_most_frequent(counter, n):
    for i, (word, number) in enumerate(counter.most_common(n), start=1):
        print "%d. %s -> %d" % (i, word, number)

def Main():
    movie = "interstellar"
    print "The Movie: " + movie
    pretty_print_list_of_n_most_frequent(Counter(split_text(extract_script_from_soup(get_soup_at_url(get_url_for_movie(movie))))), 20)

if __name__ == "__main__":
    Main()

Some might consider the line in Main too long. I'll let you split it the way you want.

A last detail

main would be a more pythonic name than Main.

\$\endgroup\$
1
2
\$\begingroup\$

One more thing

I would replace

    text = re.sub(r'\W+', ' ', text) #remove puncation from the text
    return text.split()

With

   return re.findall('\w+', text) #find all words in a text
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Regular expressions should always be raw strings. re.findall(r'\w+', text) \$\endgroup\$ – IceArdor Dec 17 '14 at 21:47
2
\$\begingroup\$

I'm starting from Josay's code with Ramast's suggestion because I don't feel like repeating their points:

from bs4 import BeautifulSoup
import requests
from collections import Counter
import operator
import re


def get_url_for_movie(movie_name):
    return "http://www.springfieldspringfield.co.uk/movie_script.php?movie={0}".format(movie_name.replace(" ", "-"))

def get_soup_at_url(url):
    r = requests.get(url)
    return BeautifulSoup(r.text)

def extract_script_from_soup(soup):
    return soup.findAll('div', {"class":"scrolling-script-container"})[0].text

def split_text(text):
    return re.findall(r'\w+', text.lower())

def pretty_print_list_of_n_most_frequent(counter, n):
    for i, (word, number) in enumerate(counter.most_common(n), start=1):
        print "%d. %s -> %d" % (i, word, number)

def main():
    movie = "interstellar"
    print "The Movie: " + movie
    pretty_print_list_of_n_most_frequent(Counter(split_text(extract_script_from_soup(get_soup_at_url(get_url_for_movie(movie))))), 20)

if __name__ == "__main__":
    main()

First, order your imports and remove pointless ones:

import re
import requests

from bs4 import BeautifulSoup
from collections import Counter

Consider get_url_for_movie. The line is rather long; it makes sense to break it up:

def get_url_for_movie(movie_name):
    url_base = "http://www.springfieldspringfield.co.uk/movie_script.php?movie="
    return url_base + movie_name.replace(" ", "-")

or

def get_url_for_movie(movie_name):
    url_base = "http://www.springfieldspringfield.co.uk/movie_script.php?movie={}"
    return url_base.format(movie_name.replace(" ", "-"))

This corresponds to the second line in your __init__.

For a larger script, it would be wise to avoid hard-coding the URL.

Consider get_soup_at_url, or get_text in the original. You don't handle errors. Here's the breif rundown on what errors are thrown. You don't need to catch these errors directly in the function, but you should probably deal with them somewhere.

Note that you should also check the response code. Often the sensible thing is just to run r.raise_for_status() to turn them into errors.

Consider extract_script_from_soup or the first line of the original parse_text. You do findAll(...)[0], but you should just use find to get the first.

Consider pretty_print_list_of_n_most_frequent. This is the bottom part of the original Main. You originally used string concatenation, but this was changed in Josay's answer to old style string formatting. Since you previously use new style string formatting, it should be so here as well:

print "{}. {} -> {}".format(i, word, number)

The new main function is too condensed (I realize this does not reflect much on the original code). I would change it to:

def main():
    movie = "interstellar"
    print "The Movie:", movie

    script = extract_script_from_soup(get_soup_at_url(get_url_for_movie(movie)))
    print_most_frequent(split_text(script), n=20)

and change pretty_print_list_of_n_most_frequent to

def print_most_frequent(items, n):
    for i, (word, number) in enumerate(Counter(items).most_common(n), start=1):
        print "{}. {} -> {}".format(i, word, number)

I would also personally remove the get_ (and extract_) prefixes unless there is an actual ambiguity. I would change split_text to words_from_text. Ignoring error handling, this gives:

import re
import requests

from bs4 import BeautifulSoup
from collections import Counter


def url_for_movie(movie_name):
    url_base = "http://www.springfieldspringfield.co.uk/movie_script.php?movie={}"
    return url_base.format(movie_name.replace(" ", "-"))

def soup_at_url(url):
    r = requests.get(url)
    r.raise_for_status()
    return BeautifulSoup(r.text)

def script_from_soup(soup):
    return soup.find('div', {"class":"scrolling-script-container"}).text

def words_from_text(text):
    return re.findall(r'\w+', text.lower())

def print_most_frequent(items, n):
    for i, (word, number) in enumerate(Counter(items).most_common(n), start=1):
        print "{}. {} -> {}".format(i, word, number)

def main():
    movie = "interstellar"
    print "The Movie: " + movie

    script = script_from_soup(soup_at_url(url_for_movie(movie)))
    print_most_frequent(words_from_text(script), n=20)

if __name__ == "__main__":
    main()
\$\endgroup\$
1
\$\begingroup\$

collections.Counter already has a method, most_common, that sorts its items from most common to least common.

def get_all_occurences(self):
    sorted_occurences = self.find_all_occurences().most_common()
    return sorted_occurences

If you need this in order from least common to most common, use reversed(Counter().most_common()).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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