# Filtering logic

I have a FileAsset class. I have a set of 'filters' with which I determine if a FileAsset should be written to my database. Below is a telescoped version of how the filters work now. My big question is this: Is there a better way? Any advice would be awesome!

(By the way, the Attribute key in the filters allows me to test other attributes of the FileAsset class which I have not demonstrated for simplicity's sake, but the concept is the same.)

import os
from fnmatch import fnmatch
import operator

class FileAsset:
def __init__(self, filename):
self.filename = filename

@property
def is_asset(self):
filters = [
{'Test':'matches','Attribute':'filename','Value':'*'},
{'Test':'ends with','Attribute':'filename','Value':'txt'},
{'Test':'does not end with','Attribute':'filename','Value':'jpg'}]
results = []
try:
results.append(file_filter(self, filters))
except Exception as e:
results.append(True)
return True in results

def __repr__(self):
return '<FileAsset: %s>' % self.filename

def file_filter(file_asset,filters):
results = []
for f in filters:
try:
attribute = operator.attrgetter(f['Attribute'])(file_asset)
try:
result = filter_map(f['Test'])(attribute,f['Value'])
results.append(result)
except Exception as e:
print e
results.append(False)
except AttributeError as e:
print e
results.append(False)
return not False in results

def filter_map(test):
if test == u'is file':
return lambda x, y: os.path.isfile(x)
elif test == u'contains':
return operator.contains
elif test == u'matches':
return lambda x, y: fnmatch(x,y)
elif test == u'does not contain':
return lambda x, y: not y in x
elif test == u'starts with':
return lambda x, y: x.startswith(y)
return lambda x, y: not x.startswith(y)
elif test == u'ends with':
return lambda x, y: x.endswith(y)
elif test == u'does not end with':
return lambda x, y: not x.endswith(y)
# etc etc

fa1 = FileAsset('test.txt')
fa2 = FileAsset('test.jpg')

print '%s goes to db: %s' % (fa1.filename, fa1.is_asset)
print '%s goes to db: %s' % (fa2.filename, fa2.is_asset)


You have written a small DSL to map tests to Python operations. The DSL uses dicts which imposes some limitations. Try this approach instead:

filter1 = FileFilter().filenameEndsWith('.txt').build()

names = ['test.txt', 'test.jpg']
to_save = filter(filter1, names)


How does that work? FileFilter is a builder. Builders are like flexible factories. Internally, the code could look like so:

class FileFilter:
def __init__(self):
self.conditions = []

def filenameEndsWith(self, pattern):
def c(filename):
return filename.endswith(pattern)
self.conditions.append(c)
return self

def build(self):
def f(filename, conditions=self.conditions):
for c in conditions:
if not c(filename): return False
return True
return f


Notes: This is just an example. You will probably use lambdas and return an object that implements the filter API in build()

import os
from fnmatch import fnmatch
import operator

class FileAsset:
def __init__(self, filename):
self.filename = filename

@property
def is_asset(self):
filters = [
{'Test':'matches','Attribute':'filename','Value':'*'},
{'Test':'ends with','Attribute':'filename','Value':'txt'},
{'Test':'does not end with','Attribute':'filename','Value':'jpg'}]
results = []
try:
results.append(file_filter(self, filters))
except Exception as e:


Don't ever do this. Python will throw exceptions in the case of many bugs in your code. If you catch all exceptions, you'll eventually catch and hide a legitimate bug. Can this even happen? Don't you catch all the interesting exceptions in file_filter?

            results.append(True)


Why do you make a list if you are only going to store a single value in it? It seems to you should just be doing return file_filter(...

        return True in results


Why do you construct a list, only to put a single item in it.

    def __repr__(self):
return '<FileAsset: %s>' % self.filename

def file_filter(file_asset,filters):
results = []
for f in filters:


I recommend spelling out filter, just for the extra clarity.

        try:
attribute = operator.attrgetter(f['Attribute'])(file_asset)


Just use attribute = getattr(file_asset, f['Attribute']), there is no point in introducing operator here.

            try:
result = filter_map(f['Test'])(attribute,f['Value'])


This really looks like filter_map should take three arguments, and not be split across two function calls

                results.append(result)
except Exception as e:


If something goes wrong, do you really want to carry on as though nothing has happened? If the requested filter was invalid in some way an exception should be raised

                print e
results.append(False)


And why in the yellow tarnekey chicken is this False, when last time an exception made it True?

        except AttributeError as e:
print e
results.append(False)


This is more acceptable, because you are attempting to specifically catch one exception, that of the attrgetter above. But you should the rest of the logic into an else block on the exception. But again, you should really throw an exception to indicate the filter was invalid.

    return not False in results


Instead use, return all(results)

def filter_map(test):
if test == u'is file':
return lambda x, y: os.path.isfile(x)


This would be a great place to use a dictionary. Have the dictionary from the test strings to the lambdas, and then you look them up.

Here is my really big problem with your code. You've build this filtering logic system and created something harder to use, less powerful, and slower then just using python.

Consider:

@property
def is_asset(self):
return (
fnmatch(self.filename, '*'),
and self.filename.endswith('txt'),
and not self.filename.endswith('jpg')
)


It seems to me that this is: shorter, easier to follow, and more flexible then your system. And that's not even considering a better way to implement this

@property
def extension(self):
return os.path.splitext(self.filename)[-1]

@property
def is_asset(self):
return self.extension == 'txt'


So I can't see how your system makes any sort of sense. But perhaps it makes more sense in the broader context of your system. If so, I'd like to see more of the broader context because I have trouble believing it.

• Thanks Winston. Sorry I'm not able to include the broader context of the problem. I understand it is difficult to provide more help without it. That being said, I learned a few things from your post. Appreciate it! – MFB Jul 16 '12 at 0:59