Class for comparing objects in Python

Question

Im curious how to improve this code. I don't like creating objects of four different types of reporters, because this could provide some problems with testing. How about the rest of this code, for example the if-statement below?

EDIT: This is a fragment of application to measure loading time of our website and few other competitors. If our site is slower than one of competitors application should send email, if our site is two times slower then application should send message via sms.

"""Module containing classes for comparing objects"""
import senders
import reporters
class Comparator:
 """Class for comparing objects"""
 def compare(self, compared, competitors):
 reporter = reporters.Reporter()
 rules = ComparatorExtensions(compared, competitors, reporter)
 send_with_email = senders.SendWithEmail()
 send_with_sms = senders.SendWithSms()
 save_to_file = senders.SendToFile()
 show_on_console = senders.ShowOnConsole()
 if rules.rule_valid_compared():
 if rules.rule_compared_smaller():
 send_with_email.send(reporter.format_report())
 if rules.rule_compared_two_times_smaller():
 send_with_sms.send(reporter.format_report())
 report = reporter.format_report()
 save_to_file.send(report)
 show_on_console.send(report)
class ComparatorExtensions:
 """Class for extensions for comparator class"""
 def __init__(self, compared, competitors, reporter):
 self.compared = compared
 self.competitors = (item for item in competitors if self.valid_item(item))
 self._reporter = reporter
 def valid_item(self, item):
 result = 'loading_time' in item
 if not result:
 self._reporter.report_invalid(item)
 return result
 def rule_compared_smaller(self):
 result = False
 for item in self.competitors:
 self._reporter.report_loading_time(item)
 if item.get('loading_time') < self.compared.get('loading_time'):
 self._reporter.report_smaller(self.compared, item)
 result = True
 return result
 def rule_compared_two_times_smaller(self):
 result = False
 for item in self.competitors:
 if (item.get('loading_time') * 2) <= self.compared.get('loading_time'):
 self._reporter.report_smaller_two_times(self.compared, item)
 result = True
 return result
 def rule_valid_compared(self):
 rule_valid = self.valid_item(self.compared)
 if rule_valid:
 self._reporter.report_loading_time(self.compared)
 return rule_valid

Full repository: https://github.com/adbo/benchmark

• Mast: Welcome to Code Review! Can you provide example usage or a description of what it's supposed to do? You've included your repository, but questions are supposed to be as stand-alone as possible. Besides, the current README doesn't explain much either. Thanks!
• Adam Borowski: This is a fragment of application to measure loading time of our website and few other competitors. If our site is slower than one of competitors application should send email, if our site is two times slower then application should send message via sms.

Answer 1

Use functions whenever possible

I think there are too many classes and you should consider using functions instead: it is easier to test (unit testing).

Use a logger

First thing: the code below make me think that you should use a logger instead of reinventing the wheel:

report = reporter.format_report()
save_to_file.send(report)
show_on_console.send(report)

Here is how you can define a root logger which can log in the console as well as in a file:

import logging
def setup_logger(log_dir, log_file_name):
 # root logger
 logger = logging.getLogger()
 formatter = logging.Formatter(
 "%(asctime)s [%(threadName)-12.12s] [%(levelname)-5.5s] %(message)s")
 file_hdlr = logging.FileHandler(
 "{0}/{1}".format(log_dir, log_file_name))
 file_hdlr.setFormatter(formatter)
 logger.addHandler(file_hdlr)
 console_hdlr = logging.StreamHandler()
 console_hdlr.setFormatter(formatter)
 logger.addHandler(console_hdlr)
 logger.setLevel(logging.DEBUG)

This setup_logger function should be called at program startup, in your main, for instance:

if __name__ == "__main__":
 setup_logger("my/work/dir", "my_app.log")

You can use this logger anywhere: in each Python module, you can define a global variable named LOG, and use it like this:

LOG = logging.getLogger(__name__)
def my_function():
 LOG.info("log message")

See the How To documentation

wrong usage of generator

In your, competitors is a generator, not a tuple. After having iterate a generator the first time, then the generator is exhausted. It means that the second loop will end immediately.

self.competitors = (item for item in competitors if self.valid_item(item))

The call to rule_compared_smaller will iterate the competitors, then the call to rule_compared_two_times_smaller will do nothing because the competitors is exhausted.

You need to create a list:

self.competitors = [item for item in competitors if self.valid_item(item)]

Answer 2

According to pep8 imports should be ordered alphabetically so

import reporters
import senders

Your classes from the senders module don't take instantiation parameters and just have one method used so these could probably just be functions.

from senders import send_email, send_sms, save_file, show_console

Similarily Comparator only contains a single method and no state so it too should be a function

def compare(compared, competitors):
 ...

valid_item and rule_valid_compared seem to have their responsibilities a bit mixed, I would change this to.

def is_item_valid(self, item):
 return 'loading_time' in item
def report_valid_item(self, item):
 is_valid = self.is_item_valid(item):
 if is_valid:
 self._reporter.report_loading_time(item)
 else:
 self._reporter.report_invalid(item)
 return is_valid

You can also reduce a lot of duplication between the two actual comparison methods by parametrize it, also as each item has already been validated you can replace the get with key lookup.

def compare_is_smaller(self, n=1, report_func=lambda a: None):
 for item in self.competitors:
 if (item['loading_time'] * n) < self.compared['loading_time']:
 report_func(self.compared, item)
 result = True
 return result
def compare_smaller(self):
 return self.compare_is_smaller(report_func=self._reporter.report_smaller)
def compare_twice_as_small(self):
 return self.compare_is_smaller(report_func=self._reporter.report_twice_as_small)