1
\$\begingroup\$

I have a large function that performs a search over a data set given a set of parameters. This function goes through multiple databases which all hold data that would comprise one final entry from the search.

def perform(self):
    #Go through each year and each DB in that year
    for year, yearInfo in data["preprocessDBInfo"].items():
        termsToCases = {}
        for dbName, dbInfo in yearInfo["dbs"].items():
            #Relevant to search?
            relevantTerms = self.search.ofDB(dbName)
            if len(relevantTerms) == 0:
                continue #Not a relevant database

            #Open the database and get cases
            fp = os.path.join(prefs["rootPath"], dbInfo["filePath"])
            caseDB = caseDB(fp)

            cases = caseDB.getCases(stubs=True,search=relevantTerms)
            termsToCases.update(cases)

        self.foundCases = self.foundCases.union(self.resolve(termsToCases))

Anyone who wants to call this function would have to wait for the entire search to complete which can take a couple of minutes depending on the search complexity.

Now I need to move this task to a separate thread of execution and be able to periodically check it and more drastically pause or kill it. In order to do this I need to break it up so that perform only does small work units so the caller of it has much more control over execution. I have come up with the following to solve this

def performGenerator(self):
    #Go through each year and each DB in that year
    for year, yearInfo in data["preprocessDBInfo"].items():
        for dbName, dbInfo in yearInfo["dbs"].items():
            #Relevant to search?
            relevantTerms = self.search.ofDB(dbName)
            if len(relevantTerms) == 0:
                continue #Not a relevant database

            #Open the database and get cases
            fp = os.path.join(prefs["rootPath"], dbInfo["filePath"])
            caseDB = caseDB(fp)
            cases = caseDB.getCases(stubs=True,search=relevantTerms)
            yield cases
  • Is there any sort of name for a function like this, a function that originally was large and autonomous but now only does tiny work units? (disregarding the fact that it's a generator in Python)
  • Is there any sort of pattern or methodology for breaking up a large subtask list this? Python's generator interface seems like it would be the best fit for my needs but I don't know what I would do if I didn't have such a high level interface (such as if I were programming in C)
\$\endgroup\$
  • \$\begingroup\$ I don't think this entirely addresses what you're asking, but refactoring to have functions address the Single Responsibility Principle does what you need. (though the principle isn't primarily concerned with interruptability) \$\endgroup\$ – SuperBiasedMan Jan 13 '16 at 10:01
  • \$\begingroup\$ @SuperBiasedMan The class that houses perform is only responsible for the search (holds the search terms (a different class) and passes them to the database class which performs the actual comparisons and such). Perform is also the only relevant function to outside callers currently. I don't see what portion I would refactor other than possible the prefs and data globals? Is there anything that jumped out in particular? \$\endgroup\$ – Coburn Jan 13 '16 at 11:00
  • \$\begingroup\$ I have a suggestion, I'll demonstrate it in an answer because I have another thought anyway. \$\endgroup\$ – SuperBiasedMan Jan 13 '16 at 11:03
1
\$\begingroup\$

You could split up your code further by separating out the lines where you actually get the cases to yield. It's only a few lines that need just a couple parameters to run and can return a simple variable, making them a prime candidate for refactoring into a function:

def performGenerator(self):
    #Go through each year and each DB in that year
    for year, yearInfo in data["preprocessDBInfo"].items():
        for dbName, dbInfo in yearInfo["dbs"].items():
            #Relevant to search?
            relevantTerms = self.search.ofDB(dbName)
            if len(relevantTerms) == 0:
                continue #Not a relevant database

            yield get_cases(relevantTerms, dbInfo["filePath"])

def get_cases(relevantTerms, filepath)
    fp = os.path.join(prefs["rootPath"], filepath)
    caseDB = caseDB(fp)
    return caseDB.getCases(stubs=True, search=relevantTerms)

This line is confusing me:

caseDB = caseDB(fp)

This isn't intended to modify caseDB, is it? As far as I can tell, it would modify it by replacing the name in your first case, but not the second since you're only modifying it inside the local scope each time the generator is called. It is a very bad idea to match names like this, because it makes it unclear whether or not they're supposed to be the same thing, or caseDB is an instance of a caseDB object. If this reuse of the name is necessary, it's a very confusing pattern that ought to be explicitly explained rather than just used without comment.

You shouldn't test for an empty collection using len(var) == 0. You're wasting time calculating the full length of something just to check if it's zero. Instead, use Python's truthiness. Any variable can be evaluated as a boolean in Python. For collections, this means that it's False if the collection is empty, and True otherwise. This means your test could be:

relevantTerms = self.search.ofDB(dbName)
if not relevantTerms:
    continue  # Not a relevant database

You don't actually use year, so you should throw it away. It's Pythonic to use an underscore for the name of a throwaway value, like this:

for _, yearInfo in data["preprocessDBInfo"].items():

This signals clearer that the value isn't used, which makes your code clearer to read.

\$\endgroup\$
  • \$\begingroup\$ Hmm, the caseDB = caseDB(fp) seems to be a typo. it should have been caseDB = CaseDB(fp) Perhaps I should change the variable name to not rely on the case sensitivity there. Thanks for the answer. \$\endgroup\$ – Coburn Jan 13 '16 at 11:57
  • \$\begingroup\$ @Coburn Yes, I did wonder if perhaps it was that. This is why using identical names with case differences is unreliable, and less readable. \$\endgroup\$ – SuperBiasedMan Jan 13 '16 at 11:58

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.