# Chain Responsibility Pattern to filtering messages

Is this a good implementation of a Chain Responsibility Pattern? This code attempts to filter a message received (for instance, in a chat application).

import urlmarker
import re

for entry in file:
entry = entry.strip()

class Message(object):

def __init__(self, content):
self.content = content.split()
self.construct_word()

def construct_word(self):
self.content = ' '.join(self.content)

class Filter():

def __init__(self, successor = None):
self.successor = successor

def has_successor(self):
if self.successor is not None:
return True
return False

def filter_message(self, message):
if self.has_successor():
self.successor.filter_message(message)

class Slang_Filter(Filter):

def filter_message(self, message):

for word in message.content:
break

if self.has_successor():
self.successor.filter_message(message)
else:

class Website_Filter(Filter):

def filter_message(self, message):
has_websites = False

if re.findall(urlmarker.URL_REGEX, message.content) is None:
if self.has_successor():
self.successor.filter_message(message)
else:

def main():
new_message = Message("This is full of fucking shit www.google.com")
slang_filter = Slang_Filter()
website_filter = Website_Filter(slang_filter)
website_filter.filter_message(new_message)

if __name__ == '__main__':
main()

• Do you really think you need the chain of responsibility pattern here? The point of chain of responsibility is creating hierarchies. If you have website filter isn't what you need is just all(predicate(message) for predicate in predicates). Also use comprehensions instead of creating empty lists and then appending to them in for loop. – user1685095 May 3 '17 at 18:55

# Chain of Responsibility

• Rather than building the chain on instantiation, make the creation of the chain a function. This can allow you to chain the creation, which is easier to read and nicer to look at.

• I'd also make the chain return, rather than mutate a foreign object. This removes side effects, which lead to hard to understand code, and potential bugs.

• has_successor is a poor function, it's simpler to read and understand using if self._next is not None, rather than using that abstraction. And so I'd remove it, I'd favour writing out the non-function way anyway.

And so I'd come to:

class Filter(object):
_next = None
self._next = next_filter
return next_filter

def filter_message(self, message):
if self._next is None:
return True
return self._next.filter_message(message)

class SlangFilter(Filter):
def filter_message(self, message):
for word in message.content:
return False

if self._next is None:
return True
return self._next.filter_message(message)

class Website_Filter(Filter):
def filter_message(self, message):
if re.findall(urlmarker.URL_REGEX, message.content) is not None:
return False

if self._next is None:
return True
return self._next.filter_message(message)


This then lets you create the chain as:

def main():
new_message = Message("This is full of fucking shit www.google.com")
filter = WebsiteFilter()
print filter.filter_message(new_message)


Going forward, you'd want to change your classes in a couple of other ways too.

• Rather than relying on a global bad words variable, SlangFilter should be passed these words on instantiation.
• Rather than manually re-creating in, you can use it to reduce code complexity, and for a performance boost, if you were to change the list of words to a set.
• Your code shouldn't rely on an unneeded model, as a string is good enough for this.

And so I'd use:

import urlmarker
import re

class Filter(object):
_next = None
self._next = next_filter
return next_filter

def filter_message(self, message):
if self._next is None:
return True
return self._next.filter_message(message)

class SlangFilter(Filter):

def filter_message(self, message):
for word in message.lower().split():
return False

if self._next is None:
return True
return self._next.filter_message(message)

class Website_Filter(Filter):
def filter_message(self, message):
if re.findall(urlmarker.URL_REGEX, message) is not None:
return False

if self._next is None:
return True
return self._next.filter_message(message)

def main():
bad_words = [word.strip() for word in file]

message = "This is full of fucking shit www.google.com"
filter = WebsiteFilter()
print filter.filter_message(message)

if __name__ == '__main__':
main()

• I found a bug in my code that was also passed on to your code: if re.findall(urlmarker.URL_REGEX, message) is not None: should be: if not re.findall(urlmarker.URL_REGEX, message): Or else, it will always return not None – TomasCarvalho May 2 '17 at 18:04
• @TomasCarvalho If you found a bug post it as an answer, and you should get upvotes, :) – Peilonrayz May 2 '17 at 18:06
• I think that your implementation (or is it me that is failing to implement it correctly...) is wrong. When you add SlangFilter, you are returning it. As such, when we call the method filter.filter_message(message) we are only using SlangFilter, as it starts with SlangFilter and its _successor is None. As such, WebFilter is never used. I'm trying to correct this bug, but it's tricky... I hope I'm making myself clear :) – TomasCarvalho May 2 '17 at 18:29
• @TomasCarvalho There's no point in calling the other functions after the first return False, and so I returned early – Peilonrayz May 2 '17 at 18:33
• The problem is with this code or the way that it's used when a filter is created: def add_filter(self, next_filter): self._next = next_filter return next_filter  Even if the message is clean (for instance, "Hello World"), only one filter will be used on it... – TomasCarvalho May 2 '17 at 18:36

After thoroughly analysing my code and @Peilonrayz answer, I came to the conclusion that there is a bug in the WebsiteFilter class. The following code will always return False:

if re.findall(urlmarker.URL_REGEX, message) is not None:
return False


The problem is that re.findall will return a list, empty or not, and as such it will never be None. It should be changed to:

if not re.findall(urlmarker.URL_REGEX, message):
return False


Other bug that I found is that in @Peilonrayz answer

filter = WebsiteFilter()

filter = WebsiteFilter()