1
\$\begingroup\$

I write an hobbyist application that collects soccer-data from different sources around the www and aggregates it for a graphical representation and sends it out as a tweet. Now I want to fully automate this process in a more or less robust way (more or less because 1. this is a hobby and 2. depends on scraped 3rd party websites).

I have problems to wrap my head around how this full automation can be organized. The following tasks need to be done:

  1. check once daily if and when there are soccer matches I am interested in
  2. start collecting data of these matches from all sources as soon as match is over
  3. retry collecting data every x minutes if it wasn't available yet (the sources have very different speed in releasing the data and it may take 24hrs. sometimes)
  4. as soon as all sources have been collected successfully launch aggregation and tweet.

My question:
I don't know how to implement the connection between 3. and 4. in a good way. I think I need some wrapper which is called by the scheduler as soon as a match is over, but this doesn't seem to be a robust way:

class MatchCollector:

    def __init__(self,teams,sources=settings.sources):
        self.teams=teams
        self.sources = sources

    def execute(self):
        finished = False
        counter = 0
        while not finished:
            finished = True
            for source in self.sources:
                s = SourceCollector(source,self.teams)
                successful = s.execute() 
                if not successful:
                    finished = False
            time.sleep(600)
            counter += counter
            if counter == 10000 # arbitrary chosen, have to make calc, when would be a good point to give up
                 raise IterationException('Tried for x hours without success. Somethings broken') 
        d = DataAggregator(sources)
        d.execute()

Am I missing something problematic, by doing this?

I arrange daily scheduling (1., 2.) via cron and python-crontab in a scheduling script. It is started once daily and reads the daily matches and writes again into cron the script to handle the single matches with arguments passed via sys.argv.

\$\endgroup\$
  • 1
    \$\begingroup\$ If you already have a working first stab at a solution, please present it above. \$\endgroup\$ – greybeard May 21 at 10:54
  • 1
    \$\begingroup\$ Does the code you present include the wrapper you mention in your question? This site is called Code Review, we look at the code and suggest improvements for the code, we can't help you write code that is not written. Please see our guidelines at codereview.stackexchange.com/help/asking. \$\endgroup\$ – pacmaninbw May 21 at 13:34
  • 1
    \$\begingroup\$ @pacmaninbw Yes, this is what I called "wrapper". As I'm self teaching coding, maybe that was not a correct terminus. But I meant the class which I showed. \$\endgroup\$ – J_Scholz May 21 at 18:27
  • 1
    \$\begingroup\$ @greybeard So I should show the whole code and not just the part I feel unsure about? \$\endgroup\$ – J_Scholz May 22 at 7:33
  • 3
    \$\begingroup\$ @J_Scholz: Yeah, that is a bit unfortunate. If I had found that problem before I had almost finished writing my answer, I probably would have just left a comment instead at first, so you would have been free to fix it. \$\endgroup\$ – Graipher May 22 at 9:49
3
\$\begingroup\$

Well, whatever you do, you should only re-check those sources which have not finished, so you should remember which have finished already, or equivalently, only those which have not.

class MatchCollector:
    def __init__(self, teams, sources=settings.sources):
        self.teams = teams
        self.sources = sources
        self.unfinished = {SourceCollector(source, teams) for source in sources}
        self.timeout = 600  # s
        self.iterations = 1000 # arbitrarily chosen, have to make calc, when would be a good point to give up

    def execute(self):
        counter = 0
        while self.unfinished:
            self.unfinished = {source for source in self.unfinished if not s.execute()}
            time.sleep(self.timeout)
            counter += 1
            if counter >= self.iterations 
                 raise IterationException('Tried for x hours without success. Somethings broken') 
        d = DataAggregator(self.sources)
        d.execute()

This assumes that SourceCollector.execute can be run repeatedly without needing to re-initialize it. If this is not the case, just do self.unfinished = {source for source in self.unfinished if not SourceCollector(source, self.teams).execute()}.

Note that doing counter += counter is probably not what you want. Since you initialize it to 0, it does 0 + 0 = 0, so nothing. If you had initialized it to a non-zero value, it would also not simply count up, but double every iteration.

I also made the timeout between retries and the number of iterations members of the class so they are no longer magic numbers.

You should also follow Python's official style-guide, PEP8, which recommends using spaces around = in assignments and after commas in argument lists of functions and methods.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Yes, should have been counter += 1, this was a mistake from the hurry I was typing. Thank you. \$\endgroup\$ – J_Scholz May 22 at 9:46
  • \$\begingroup\$ @J_Scholz: Sorry, but I rolled back that edit (but added back the text you made in your next edit) , since it invalidated parts of my answer. See the link I posted above for more explanation on what you can and cannot do here after having received an answer. \$\endgroup\$ – Graipher May 22 at 9:48
  • \$\begingroup\$ I think self.unfinished should be a list-, not a dict-comprehension? \$\endgroup\$ – J_Scholz May 22 at 10:06
  • 1
    \$\begingroup\$ @J_Scholz: A list comprehension would also work, but it is a set comprehension here, not a dict comprehension. I had originally wanted to use set.add and set.remove, which are faster for sets than lists, especially the latter if the element is not at the end of the list. As long as your class is hashable (which it is by default), a set will work just as well here, except that the order in which you try the sources may not always be the same. \$\endgroup\$ – Graipher May 22 at 10:08

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.