Tuesday, March 31, 2009

Is This Pythonic? Conclusion

I couldn't leave well enough alone and continued to refactor the code from the Is This Pythonic? Open Space. The final version I submitted as a patch to the original project. The biggest change was not in apply_all (as I assumed) but in writing a new chunk of code that sucks all the ugly and special cases from the rest of the code and puts it in one place. I don't know if there is a pattern name for this but it tends to happen at boundaries. Pretty print functions are usually ugly for instance, and for good reason - your only other choice is to ugly up the core.

So here is FetchAccumulator. It sits at the boundry just above the database calls and returns tidy, regular data to its callers.

class FetchAccumulator(object):
def __init__(self, sql, args=None, fetch_per=-1, limit=-1):
self.results = []
self.sql = sql
self.args = args
self.fetch_per = fetch_per
self.limit = limit
return

def fetch(self, cursor):
cursor.execute(self.sql, self.args)
if self.fetch_per == 1:
results = cursor.fetchone()
assert len(results) <= 1, results
elif self.limit > 0:
results = cursor.fetchmany(self.limit)
assert len(results) <= self.limit, (len(results), self.limit)
else:
results = cursor.fetchall()

if not results or not filter(None, results): # code smell
return

self.results.extend(results)
self.limit -= len(results)

if not self.limit: # we fetched our limit
raise DoneApply()
return

def __iter__(self):
return iter(self.results)

This makes the other functions much, much simpler. Here are four database query functions that use FetchAccumulator. Seventy lines are now twenty.

class ShardCursor(cursor.BaseCursor):
def selectOne(self, sql, args=None):
accum = FetchAccumulator(sql, args, fetch_per=1, limit=1)
apply_all(valid_shards(self._shard), accum.fetch)
return accum

def selectMany(self, sql, args=None, size=-1):
accum = FetchAccumulator(sql, args, limit=size)
apply_all(valid_shards(self._shard), accum.fetch)
return accum

def selectAll(self, sql, args=None):
accum = FetchAccumulator(sql, args)
apply_all(valid_shards(self._shard), accum.fetch)
return accum

def countOne(self, sql, args=None):
accum = FetchAccumulator(sql, args, fetch_per=1)
apply_all(valid_shards(self._shard), accum.fetch)
return accum

Of course these functions now have their own code smell -- they only vary in their accumulator so they could be collapsed into a single function. That would require refactoring all the calling code which is a bigger project than I wanted to take on.

The apply_all function grew a proper exception to allow callers to bail out of the loop early.

class DoneApply(Exception): pass

def apply_all(shards, func):
for shard in shards:
db = shard.establishConnection()
try:
cursor = db.cursor()
func(cursor)
except DoneApply:
break
finally:
db.close()


I'll omit the unit tests. The original project had no unit tests for this code so I had to write some to make sure my refactoring wasn't breaking anything.

No comments: