We'll be counting stars

Question

Lately, I've been, I've been losing sleep
Dreaming about the things that we could be
But baby, I've been, I've been praying hard,
Said, no more counting dollars
We'll be counting stars, yeah we'll be counting stars
(One Republic - Counting Stars)

The 2nd Monitor is known to be a quite star-happy chat room, but exactly how many stars is there? And who are the most starred users? I decided to write a script to find out.

I chose to write this in Python because:

• I've never used Python before
• @200_success used it and it didn't look that hard
• I found Beautiful Soup which seemed powerful and easy to use

The script performs a number of HTTP requests to the list of starred messages, keeps track of the numbers in a dict and also saves the pure HTML data to files (to make it easier to perform other calculations on the data in the future, and I had the opportunity to learn file I/O in Python).

To be safe, I included a little delay between some of the requests (Don't want to risk getting blocked by the system)

Unfortunately there's no way to see which user starred the most messages, but we all know who that is anyway...

The code:

from time import sleep
from bs4 import BeautifulSoup
from urllib import request
from collections import OrderedDict
import operator

room = 8595 # The 2nd Monitor (Code Review)
url = 'http://chat.stackexchange.com/rooms/info/{0}/the-2nd-monitor/?tab=stars&page={1}'
pages = 125

def write_files(filename, content):
    with open(filename, 'w', encoding = 'utf-8') as f:
        f.write(content)

def fetch_soup(room, page):
    resource = request.urlopen(url.format(room, page))
    content = resource.read().decode('utf-8')
    mysoup = BeautifulSoup(content)
    return mysoup

allstars = {}
def add_stars(message):
    message_soup = BeautifulSoup(str(message))
    stars = message_soup.select('.times').pop().string
    who = message_soup.select(".username a").pop().string

    # If there is only one star, the `.times` span item does not contain anything
    if stars == None:
        stars = 1

    if who in allstars:
        allstars[who] += int(stars)
    else:
        allstars[who] = int(stars)

for page in range(1, pages):
    print("Fetching page {0}".format(page))
    soup = fetch_soup(room, page)
    all_messages = soup.find_all(attrs={'class': 'monologue'})
    for message in all_messages:
        add_stars(message)
    write_files("{0}-page-{1}".format(room, page), soup.prettify())
    if page % 5 == 0:
        sleep(3)

# Create a sorted list from the dict with items sorted by value (number of stars)
sorted_stars = sorted(allstars.items(), key=lambda x:x[1])

for user in sorted_stars:
    print(user)

The results, you ask? Well, here they are: (Spoiler warning!) (I'm only showing those who have \$> 50\$ stars here, to keep the list shorter)

('apieceoffruit', 73)
('ChrisW', 85)
('Edward', 86)
('Yuushi', 93)
('Marc-Andre', 98)
('nhgrif', 112)
('amon', 119)
('James Khoury', 126)
('Nobody', 148)
('Jerry Coffin', 150)
('BenVlodgi', 160)
('Donald.McLean', 174)
('konijn', 184)
('200_success', 209)
('Vogel612', 220)
('kleinfreund', 229)
('Corbin', 233)
('Morwenn', 253)
('skiwi', 407)
('lol.upvote', 416)
('syb0rg', 475)
('Malachi', 534)
('retailcoder', 749)
("Mat's Mug", 931)
('Simon André Forsberg', 1079)
('Jamal', 1170)
('The Mug with many names', 2096) (Mat's Mug, retailcoder and lol.upvote is the same user)
('rolfl', 2115)

It feels strange to do .pop() to fetch data from the select soup, is there another approach available here? However, as this is the first time I'm using Python, any comments are welcome.

Answer 1

That's a beautiful excuse for writing code, and the final product is quite nice as well.

★ First of all, congrats to your Java-ridden mind for not forcing classes into Python where they aren't needed.

★ You import but do not use OrderedDict and operator. Remove unused imports.

★ It is customary to write your code in a way that allows it to be used as either a module, or a script. For this, the if __name__ == '__main__' trick is used:

if __name__ == '__main__':
    # executed only if used as a script

★ You declare a few variables like room, url, and pages up front. This hinders code reuse:

• The room variable should not be declared up front as a global variable, but in your main section. From there, it can be passed down through all functions.

• The url specifically but unnecessarily mentions the-2nd-monitor. This isn't harmful, but unnecessary as only the ID is relevant. Furthermore, url is an awfully short name for such a large scope. Something like star_url_pattern would be better – except that global “constants” should be named all-uppercase:

  STAR_URL_PATTERN = 'http://chat.stackexchange.com/rooms/info/{0}/?tab=stars&page={1}'
  

• Reserve plural names for collections. pages should rather be page_count. But wait – why are we hardcoding this rather than fetching it from the page itself? Just follow the rel="next" links until you reach the end.

★ That last idea could be implemented with a generator function. A Python generator function is similar to a simple iterator. It can yield elements, or return when exhausted. We could build such a generator function that yields a beautiful soup object for each page, and takes care of fetching the next one. As a sketch:

from urllib.parse import urljoin

def walk_pages(start_url):
    current_page = start_url
    while True:
        content = ... # fetch the current_page
        soup = BeautifulSoup(content)
        yield soup
        # find the next page
        next_link = soup.find('a', {'rel': 'next'})
        if next_link is None:
            return
        # urljoin takes care of resolving the relative URL
        current_page = urljoin(current_page, next_link.['href'])

★ Please don't use urllib.request. That library has a horrible interface and is more or less broken by design. You might notice that the .read() method returns raw bytes, rather than using the charset from the Content-Type header to automatically decode the content. This is useful when handling binary data, but a HTML page is text. Instead of hardcoding the encoding utf-8 (which isn't even the default encoding for HTML), we could use a better library like requests. Then:

import requests
response = requests.get(current_page)
response.raise_for_status()  # throw an error (only for 4xx or 5xx responses)
content = response.text  # transparently decodes the content

★ Your allstars variable should not only be named something like all_stars (notice the separation of words via an underscore), but also not be a global variable. Consider passing it in as a parameter to add_stars, or wrapping this dictionary in an object where add_stars would be a method.

★ I don't quite understand why you write each page to a file. I suspect this was intended as a debugging help, but it doesn't add any value to a user of that script. Instead of cluttering the current working directory, make this behavior optional.

★ Do not compare to None via the == operator – this is for general comparison. To test for identity, use the is operator: if stars is None. Sometimes, it is preferable to rely on the boolean overload of an object. For example, arrays are considered falsey if they are empty.

---

Talking is easy,
coding is hard.
Is this refactoring
equally bad?

import time
from bs4 import BeautifulSoup
import requests
from urllib.parse import urljoin

STAR_URL_TEMPLATE = 'http://chat.{0}/rooms/info/{1}/?tab=stars'

def star_pages(start_url):
    current_page = start_url
    while True:
        print("GET {}".format(current_page))
        response = requests.get(current_page)
        response.raise_for_status()
        soup = BeautifulSoup(response.text)
        yield soup
        # find the next page
        next_link = soup.find('a', {'rel': 'next'})
        if next_link is None:
            return
        # urljoin takes care of resolving the relative URL
        current_page = urljoin(current_page, next_link['href'])

def star_count(room_id, site='stackexchange.com'):
    stars = {}
    for page in star_pages(STAR_URL_TEMPLATE.format(site, room_id)):
        for message in page.find_all(attrs={'class': 'monologue'}):
            author = message.find(attrs={'class': 'username'}).string

            star_count = message.find(attrs={'class': 'times'}).string
            if star_count is None:
                star_count = 1

            if author not in stars:
                stars[author] = 0
            stars[author] += int(star_count)

        # be nice to the server, and wait after each page
        time.sleep(1)
    return stars

if __name__ == '__main__':
    the_2nd_monitor_id = 8595
    stars = star_count(the_2nd_monitor_id)
    # print out the stars in descending order
    for author, count in sorted(stars.items(), key=lambda pair: pair[1], reverse=True):
        print("{}: {}".format(author, count))

Answer 2

I can't see any reason why you would need to pop elements off the arrays returned from .select() -- you could just do something like

 message_soup.select('.times')[0].string

Either approach will throw an exception if the message doesn't contain a .times class, so you could add some exception handling:

try:
    stars = message_soup.select('.times')[0].string
except IndexError:
    stars = None

Having said that, I don't think it's particularly wrong to use a .pop() -- it depends other factors that may come down to judgement. A co-worker I respect seems to think using pop() in python is a bit unpythonic. Another coworker who is more Lisp inflected likes it. Personally, I think I would leave it out, unless there was something compelling about mutating the data structure.

Answer 3

There are also some possible BeautifulSoup related improvements:

• It is highly recommended to specify an underlying parser that BeautifulSoup would use under the hood:

  soup = BeautifulSoup(response.text, "html.parser")
  # soup = BeautifulSoup(response.text, "lxml")
  # soup = BeautifulSoup(response.text, "html5lib")
  

  If you don't specify a parser, BeautifulSoup would pick one automatically from what is available in the current Python environment - and it may work differently on different machines and environments leading to surprising consequences. See also Installing a parser documentation section.

• Instead of doing .select() and .pop(), you can call .select_one() method directly which would return a single element or None (if no elements found)
• The soup.find_all(attrs={'class': 'monologue'}) can be replaced with a more concise CSS selector call: soup.select('.monologue')