Two Python classes to update an RRD database

Question

The first Python class below is used in one of my current projects and is a totally different approach to how I normally would take things. Before I refactored the class I was not declaring instance-properties/variables. Instead the whole update procedure took place inside the def update(self): method (see: "The 'old style' class"). All those instance-variables where defined inside the scope of def update(self):.

This leaded to a class with a few less lines but was harder to read so I decided to split things into "areas of responsibilities" and defined separate functions/methods (See "New Style Class" - first code snippet).

I really like the new code much more than before. It's better to read and everything is taken very short and has it's own area of responsibility.

My concerns however are, that this is "too much" or not neccesarry and might lead to problems with memory management in python - which should not be the case since the ARC should recognize assigning new values and release the old one, right!?

Anyway, just leave some feedback so I can decide if I totally adapt this kind of "my new style" I am working on!

The First "New Style" Class

from AlphasenseOPCN2Wrapper import AlphasenseOPCN2Wrapper
from RRDLongtermDatabase import RRDLongtermDatabase
from RRDRealtimeDatabase import RRDRealtimeDatabase
from ValueTransformation import PMValueTransformationExtractor
from ValueTransformation import BinValueTransformationExtractor
class DatabaseUpdater(object):
 def __init__(self):
 self.rrd_real_time_database = RRDRealtimeDatabase()
 self.rrd_long_term_database = RRDLongtermDatabase()
 self.alphasense_opcn2_wrapper = AlphasenseOPCN2Wrapper()
 self._bin_values = None
 self._bin_sum_value = None
 self._pm_values = None
 self._histogram_value = None
 self._start_up()
 def _start_up(self):
 self.alphasense_opcn2_wrapper.start()
 def update(self):
 self._get_histogram_values()
 self._get_bin_values()
 self._get_pm_values()
 self._update_longterm_values()
 self._update_realtime_values()
 def _get_histogram_values(self):
 self.histogram_values = self.alphasense_opcn2_wrapper.histogram()
 def _get_bin_values(self):
 bin_value_transformer = BinValueTransformationExtractor(self.histogram_values)
 self.bin_values = bin_value_transformer.value
 self.bin_sum_value = sum(self.bin_values)
 def _get_pm_values(self):
 pm_value_transformer = PMValueTransformationExtractor(self.histogram_values)
 self.pm_values = pm_value_transformer.value
 def _update_longterm_values(self):
 long_term_values = self.pm_values + [self.bin_sum_value]
 self.rrd_long_term_database.update(long_term_values)
 def _update_realtime_values(self):
 real_time_values = self.pm_values + [self.bin_sum_value] + self.bin_values
 self.rrd_real_time_database.update(real_time_values)
 def __del__(self):
 self.alphasense_opcn2_wrapper.stop()
 print('Deinit Database Updater')

The "old style" class

... which I was not happy with:

class DatabaseUpdater(object):
 def __init__(self):
 self.rrd_real_time_database = RRDRealtimeDatabase()
 self.rrd_long_term_database = RRDLongtermDatabase()
 self.alphasense_opcn2_wrapper = AlphasenseOPCN2Wrapper()
 self._start_up()
 def _start_up(self):
 self.alphasense_opcn2_wrapper.start()
 def update(self):
 histogram_values = self.alphasense_opcn2_wrapper.histogram()
 bin_value_transformer = BinValueTransformationExtractor(histogram_values)
 bin_values = bin_value_transformer.value
 bin_sum_value = sum(bin_values)
 pm_value_transformer = PMValueTransformationExtractor(histogram_values)
 pm_values = pm_value_transformer.value
 long_term_values = pm_values + [bin_sum_value]
 real_time_values = pm_values + [bin_sum_value] + bin_values
 self.rrd_long_term_database.update(long_term_values)
 self.rrd_real_time_database.update(real_time_values)
 def __del__(self):
 self.alphasense_opcn2_wrapper.stop()
 print('Deinit Database Updater')

Sure, it has less lines of code but in my eyes it's harder to read.

---

In the above examples I am referencing two classes, called *ValueTransformerExtractor. I have been creating these classes in order to take more code out of a single place and put it in it's own "area of responsibility". Here's the code to these two classes:

class BinValueTransformationExtractor(object):
 def __init__(self, histogram):
 self.value = self._extract_and_transform(histogram)
 def _extract_and_transform(self, histogram_values):
 sample_flow_rate = histogram_values['SFR']
 sampling_period = histogram_values['Sampling Period']
 print "Sampling Period: " + str(sampling_period)
 bin_values = []
 transformation_multiplier = 1000000.0 / sample_flow_rate
 for index in range(0, 16):
 key = 'Bin ' + str(index)
 value = (histogram_values[key] / sampling_period) * transformation_multiplier
 bin_values.append(value)
 return bin_values
class PMValueTransformationExtractor(object):
 def __init__(self, histogram_values):
 self.value = self._extract_and_transform(histogram_values)
 def _extract_and_transform(self, histogram_values):
 pm_values = [histogram_values['PM10'],
 histogram_values['PM2.5'],
 histogram_values['PM1']]
 return map(self._apply_density, pm_values)
 def _apply_density(self, value):
 density = 2.5
 sensor_density = 1.65
 density_factor = density / sensor_density
 value *= density_factor
 return value

• Phrancis: I agree that fewer lines of code are not necessarily better, and with Python especially it's tempting to write really short code. +1, hope you get good reviews!

Answer 1

1. Review

I much prefer the "old style" version of the code. That's because there's less code to read, and the logic is easier to follow — I don't have to keep jumping back and forth among little methods which are not documented and whose purpose is not clear.

I would suggest the following changes:

1. The _start_up method is only one line long and only called from one place, so it could be inlined into __init__.

2. The BinValueTransformationExtractor class has only one method, and the purpose of the method is to compute one value, so what you actually want here is a function, not a class.

3. Similarly for PMValueTransformationExtractor.

4. It's clearer to write 1e6 rather than 1000000.0. The latter is easily confused with 100000.0 and 10000000.0.

5. range starts at 0 by default, so you can write range(16).

6. The division by sampling_period is the same every time, so could be folded into the multiplier.

7. density_factor is always the same, so it could be made into a global variable and not recomputed every time.

2. Revised code

def bin_values(histogram):
 sample_flow_rate = histogram['SFR']
 sampling_period = histogram['Sampling Period']
 multiplier = 1e6 / (sample_flow_rate * sampling_period)
 return [histogram['Bin {}'.format(i)] * multiplier for i in range(16)]
_DENSITY = 2.5
_SENSOR_DENSITY = 1.65
_DENSITY_FACTOR = _DENSITY / _SENSOR_DENSITY
_PM = (10, 2.5, 1)
def pm_values(histogram):
 return [histogram['PM{}'.format(pm)] * _DENSITY_FACTOR for pm in _PM]
class DatabaseUpdater(object):
 def __init__(self):
 self.rrd_real_time_database = RRDRealtimeDatabase()
 self.rrd_long_term_database = RRDLongtermDatabase()
 self.alphasense_opcn2_wrapper = AlphasenseOPCN2Wrapper()
 self.alphasense_opcn2_wrapper.start()
 def update(self):
 histogram = self.alphasense_opcn2_wrapper.histogram()
 pm = pm_values(histogram)
 bin = bin_values(histogram)
 bin_sum = sum(bin)
 self.rrd_long_term_database.update(pm + [bin_sum])
 self.rrd_real_time_database.update(pm + [bin_sum] + bin)
 def __del__(self):
 self.alphasense_opcn2_wrapper.stop()

• Simon Kemper: Agree. This is a very nice version of my "old style" version of the code. There are some nice things in it which I will adapt my projects. Your answer will help me improve my style and get a better understanding of python! Thanks!