Using Scrapy/Xpath to scrape ESPN for football (soccer) commentaries

Question

The class takes an input of a game-id from the ESPN soccer website. The code then has to grab the commentary, process that and also grab the player names/ids and create a small dictionary of those.

I understand the premise of OOP when shown in basic examples but I cannot get my head around using it in actual programs/applications. I have tried here, but to me it seems to not achieve anything extra (so I must be doing it wrong).

I end up with a list of lists containing each of the key events (corners, shots etc) and a dictionary of players, so it works. But it just seems so inelegant and I would love some tips on improving it (however small/focused they are).

import urllib2
from scrapy.selector import HtmlXPathSelector
import re
import codecs
import timeit
start = timeit.default_timer()

class game:
    def __init__(self,game_id):
        self.game_id = game_id
        self.comms_url = 'http://espnfc.com/uk/en/gamecast/%d/gamecast.html?soccernet=true&cc=5739' % self.game_id
        self.data_text = urllib2.urlopen(self.comms_url).read()
        self.corners = []
        self.shots = []
        self.fouls = []
        self.shots_on_target = []
        self.offsides = []
        self.goals = []
        self.extractCommentaries()
        self.extractPlayers()
        self.extractTeams()
        self.findAction('^Corner', self.corners)
        self.findAction('^Foul by', self.fouls)
        self.findAction('^Attempt', self.shots)
        self.findAction('^Attempt saved', self.shots_on_target)
        self.findAction('^Offside', self.offsides)
        self.findAction('^Goal!', self.goals)
    def extractCommentaries(self):
        self.hxs = HtmlXPathSelector(text=self.data_text)
        self.match_comments = self.hxs.select("//div[@id='convo-window']/ul[@id='convo-list']/li/div[@class='comment']/p/text()")
        self.match_timestamps = self.hxs.select("//div[@id='convo-window']/ul[@id='convo-list']/li/div[@class='timestamp']/p/text()")
        self.events = zip(self.match_timestamps.extract(), self.match_comments.extract())
        self.cleanclean = []
        for time,event in self.events:
            time = re.search('\d+', time)
            if time:
                time = time.group()
                time = int(time)
                self.cleanclean.append((time,event))
        self.dumdum = range(len(self.events))
        self.clean_events = {}
        for key, event in enumerate(self.cleanclean):
            self.clean_events[key] = event
    def extractPlayers(self):
        self.player_names = self.hxs.select("//table[@class='stat-table']/tbody//a/text()")
        self.player_urls = self.hxs.select("//table[@class='stat-table']/tbody//a/@href")
        self.player_ids = []
        for url in self.player_urls.extract():
            digit = []
            for s in url:
                if s.isdigit():
                    digit.append(s)
            self.placeholder = ''.join(digit)
            self.placeholder = int(self.placeholder)
            self.player_ids.append(self.placeholder)
        self.players = dict(zip(self.player_ids, self.player_names.extract()))
    def extractTeams(self):
        self.dummy = ['home','away']
        self.zero = [0,0]
        self.team_names = self.hxs.select("//li[@class='country']/h2/text()")
        self.teams = dict(zip(self.dummy,self.team_names.extract()))
    def findAction(self, keyword, storage):
        for key, value in self.clean_events.iteritems():
            action = re.search(keyword, value[1])
            if action:
                for ha, team in self.teams.iteritems():
                    if keyword == '^Goal!':
                        split_up_action = value[1].split('.')
                        found = re.search(team, split_up_action[1])
                    else:
                        found = re.search(team, value[1])
                    if found:
                        temp_holder = [value[0], team, self.game_id]
                        storage.append(temp_holder)

a = game()
stop = timeit.default_timer()

print stop - start

Answer 1

• There are no docstrings, so a user of your class is left wondering which methods could be useful to call. In fact, after studying the code, it looks like all the methods are meant to be called by __init__, in a specific order. Makes me wonder if this should be a class at all.
• You set very many instance attributes, even a self.dummy, but use very few outside the method where they are set. Most of them should be local variables instead. Again, I'm thinking that you don't need a class -- all the methods could be simple functions that take one or two parameters instead.
• The purpose of the class seems to be to initialize six lists, eg. self.corners. So, to get an overview of the game, one has to access six variables. It would be better to use a single data structure that can be easily looped over and queried. Perhaps a dictionary with keys like "corners" could suit you, but I'm thinking that most useful would be to keep the events in a chronological list and provide a function to filter that list by type of event.

Answer 2

I'm having trouble following this code, so I'm going to focus on maintainability. Here are some things you can consider changing:

• Some members are currently initialized in __init__, some are only initialized in other methods. It's typically good to define invariants of your class that will remain true at all observable times. Since __init__ calls everything that's still mostly true for external users, but it makes it harder for you to remember what's ready in game's methods.
• Echoing Janne Karila, some members are being used where there's no need for anything else to see them. Locals should be used instead. Maybe fixing this would remove most of the weird cases of the previous bullet.

• There's a special case in findAction for one of the many parameters it's called with. I find findAction very hard to follow. But I also don't like how it takes a list by reference and appends to it. It's typically more pythonic to return a value. If findAction created and returned a list, then your caller could be self.corners = self.findAction('^Corner'). This would even clean up some of __init__.

• There are a number of magic values mixed in with the rest of the code. Sometimes it's nice to extract these to a single location and give them a meaningful name. For example the URL format string, or the XPath strings, or even the regex pattern strings. As these are immutable values I would consider placing these on the class:

  class game:
      COMMS_URL_FMT = 'http://...%s...'
      COMMENT_XPATH = "//div/..."
  
      def __init__(self, game_id):
          self.data_text = urllib2.urlopen(self.COMMS_URL_FMT % game_id).read()