Python: feedback / corrections on first OOP style script.

Question

I would like some feedback on my first Python script that makes use of OOP style. This is a Munin plugin that graphs average fan speed or average chassis temp depending on the name of the plugin (dell_fans, dell_temps).

An hour or so ago I submitted a procedural version of the fan speed plugin to stackoverflow to get help converting it to an OOP style. I then built off of that to combine the two scripts. Any feedback, suggestions, corrections would be very helpful. I would like to correct any misconceptions I may have before they cement.

Update: Modified to have common base class. Any other suggestions?

import sys
import subprocess

class Statistics(object):

    def __init__(self, command):
        self.command = command.split()

    def average(self):
        data = subprocess.Popen(self.command,stdout=subprocess.PIPE).stdout.readlines()

        count = total = 0
        for item in data:
            if "Reading" in item:
                # Extract variable length fan speed, without regex.
                total += float(item.split(":")[1].split()[0])
                count += 1
        # Sometimes omreport returns zero output if omsa services aren't started.
        if not count or not total:
            raise ValueError("No output from omreport. Is OMSA services started?")

        avg = (total / count)
        return avg

    def print_autoconfig(self):
        print "autoconfig goes here"


class Fanspeed(Statistics):

    def __init__(self, command):
        Statistics.__init__(self, command)

    def print_config(self):
        print "graph_title Average Fan Speed"
        print "graph_args --base 1000 -l 0"
        print "graph_vlabel speed (RPM)"
        print "graph_category Chassis"
        print "graph_info This graph shows the average speed of all fans"
        print "graph_period second"
        print "data.label speed"
        print "data.info Average fan speed for the five minutes."


class Temps(Statistics):

    def __init__(self, command):
        Statistics.__init__(self, command)

    def print_config(self):
        print "graph_title Average Temperature"
        print "graph_args --upper-limit 120 -l 0"
        print "graph_vlabel Celsius"
        print "graph_category Chassis"
        print "graph_info This graph shows the avg temp of all sensors."
        print "graph_period second"
        print "data.label temp"
        print "data.info Average chassis temperature for the five minutes."


if __name__ == '__main__':
    # Munin populates sys.argv[1] with "" (an empty argument), lets remove it.
    sys.argv = [x for x in sys.argv if x]

    if "fans" in sys.argv[0]:
        cmd = "/usr/sbin/omreport chassis fans"
        omdata = Fanspeed(cmd)
    elif "temps" in sys.argv[0]:
        cmd = "/usr/sbin/omreport chassis temps"
        omdata = Temps(cmd)
    else:
        print >> sys.stderr, "Change filename to dell_fans or dell_temps."
        sys.exit(1)

    if len(sys.argv) > 1:
        if sys.argv[1].lower() == "autoconfig":
            omdata.print_autoconfig()
        elif sys.argv[1].lower() == "config":
            omdata.print_config()
    else:
        try:
            average = omdata.average()
            print "data.value %s" % average
        except OSError, e:
            print >> sys.stderr, "Error running '%s', %s" % (cmd, e)
            sys.exit(1)
        except ValueError, e:
            # Sometimes omreport returns zero output if omsa services aren't started.
            print >> sys.stderr, 'Error: "omreport chassis fans" returned 0 output.'
            print >> sys.stderr, 'OMSA running? Try: "srvadmin-services.sh status".'
            sys.exit(1)

Answer 1

Is Temps a FanSpeed? That's the litmus test for whether subclassing is appropriate (e.g. an elephant is an animal, a car is not an animal - so it might be appropriate to have a subclass of Animal which models an elephant, but not a subclass of Animal which models a car).

It sounds like they're modelling two different things - so yes, create a common base class for them.

Answer 2

A sematically more appropriate way would be to define one main class (for example, FanStatistics or whatever you want to call it), in which you define general methods, like the average method, and the subclass it into FanSpeed and FanTemp. That way you wouldn't confuse the names, as the temperature is not a subclass, or specialization, of the speed – but the speed and the temperature are both specializations of the abstract statistics data.

Answer 3

It sounds to me like you have two different kinds of statistics that you want to model, fan speed (Fanspeed) and temperatute (Temps). If there is some common functionality that you want share between them create common base class for them lets call it Statistic for example.

class Statistic(object):
    def average(self):
        pass # your logic to calculate the average here

class Fanspeed(Statistic):
    pass # your fan speed functionaly here

class Temps(Statistic):
    pass # your temperature functionaly here