Refractoring OOP vs. Functional Programming Approach

Question

I am writing a python script that will call mandrills export activity api for every blog on a worpress mu install except for the main site. The script is designed to be called from a cron job on a regular schedule. The code I have is here.

__author__ = 'Taylor'

import datetime
import mandrill
import phpserialize
import mysql.connector


class BlogReporter(object):
    blog_id = None
    table_name = 'wp_%s_options'
    notify_email = None
    sender_email = None
    date_to = None
    date_from = None

    def __init__(self, blog_id_, database_config_, mandrill_client_):
        self.blog_id = blog_id_
        self.mandrill_client = mandrill_client_
        self.table_name = 'wp_' + str(blog_id_) + '_options'
        try:
            cnx_ = mysql.connector.connect(**database_config_)
        except mysql.connector.Error as err:
            print(err)
        else:
            query_ = "SELECT option_value FROM " + self.table_name + " WHERE option_name=%s"
            try:
                cursor_ = cnx_.cursor()
                cursor_.execute(query_, ('admin_email',))
                result = cursor_.fetchone()
            except Exception as err:
                print(err)
            else:
                # set notify email from cursor
                self.notify_email = result
                cursor_.close()
            try:
                cursor_ = cnx_.cursor()
                cursor_.execute(query_, ('wysija',))
                encoded_result = cursor_.fetchone()[0]
            except Exception as err:
                print(err)
            else:
                decoded_result = phpserialize.unserialize(encoded_result.decode('base64', 'strict'))
                self.sender_email = decoded_result['from_email']
                cursor_.close()
            cnx_.close()
        self.date_to = datetime.datetime.now()
        # if it monday lets get activity since saturday
        if self.date_to.weekday() is 0:
            self.date_from = self.date_to - datetime.timedelta(days=2)
        # else just get the activity from the past day
        else:
            self.date_from = self.date_to - datetime.timedelta(days=1)

    def export_activity(self):
        self.mandrill_client.exports.activity(self.notify_email, self.date_from, self.date_to, None,
 self.sender_email,'Sent')


# database name and credentials
# these should eventually be parsed from command line arguments
database_config = {
    'user': 'root',
    'password': 'password',
    'database': 'wpmudb'
}
# Mandrill API Key
# this should also be parsed from command line
mandrill_api_key = 'xxxxx_xxxxxxxxxxxxxxxx'
mandrill_client = mandrill.Mandrill(mandrill_api_key)

try:
    cnx = mysql.connector.connect(**database_config)
except mysql.connector.Error as err:
    print(err)
else:
    try:
        cursor = cnx.cursor()
        # Get all blogs but blog_id 1 since the main site needs no report
        query = "SELECT blog_id FROM wp_blogs WHERE blog_id != '1'"
        cursor.execute(query)
    except Exception as err:
        print(err)
    else:
        for blog_id in cursor:
            blog = BlogReporter(blog_id[0], database_config, mandrill_client)
            blog.export_activity()
            del blog
        cursor.close()
    cnx.close()

I wrote it like this because it is the first program I have written in python and I was unsure of how to structure it and wanted to learn more about classes in python. However, recognizing that in the programs current state Class BlogReporter's constructer and method export_activity could be refactored into one function that accepts the arguments of class BlogReporter's constructor and is simply the class's constructor plus the method export_activity; I was curious if doing so would be beneficial to the program in any way.

I would also like to welcome comments as to whether the mandrill client should be reinstantiated for each instance of BlogReporter or whether it is better to pass the same client to each instance.

Answer 1

Here are some points:

and wanted to learn more about classes in python

If you write some piece of code to learn something, delete it and rewrite it.

construct[o]r and method export_activity could be refactored into one function

If you have a class with a single method converting to a function is what you should do by default. By by default I mean unless you have a really solid reason to do otherwise.

I was curious if doing so would be beneficial to the program in any way.

Simplicity is second only to (actually needed) functionality. (As XP people say Do the simplest thing that could possibly work.) You do not have to justify making your program simpler by appealing to a higher value.

Some more specific points:

The sections of code where you read an option from the database is the same almost word for word. If you find yourself writing the same code twice or more tellingly *Copy&Paste*ing some snippet you should put it in a function and call it instead. This way you will save yourself a lot of effort. As a general rule you should write everything once and only once.

Your constructor does too much. Anything other that assigning fields should not be done in a constructor. But your constructor is too long even for a method. Extract every snippet of code that is doing something in a function so that when you (or someone else) read it again instead of trying to guess what you are trying to do by reading how you do it you would just read what you are trying to do. This way you (or others) may skip reading the extracted function altogether if they are not interested.

Clean up of resources (files, connections, cursors etx) are usually done by initializing them in with blocks. If the resource does not support with blocks out-of-the box but cleaning-up consists of calling .close() on it, you can wrap the resource in contextlib .closing The clean-up should otherwise be done in finally blocks to ensure they are executed even if an exception is thrown; at least in long running programs, server or desktop applications.

When you decide to catch an exception, catch the most specific exception you expect. Look what else are you catching when you say catch Exception. Here root exception of all mysql.connector errors (apparently mysql.connector.Error) is more suitable.

If you are not going to do anything useful (recover, wrap&rethrow etc) do not catch exceptions. You can do print(err) somewhere else.

In a script put your code in a function, for example main, and call it if __name__ == "__main__": main(). That way you can debug your code in repl, or import it in other scripts/programs as a module. This also allows further refactorings, which I won't elaborate but you can see in the code below.

If a method has many parameters and you are calling it many times with same parameters you can use functools.partial to reduce the noise. (see below)

Also cursor.fetchone() apparently returns tuple and I'm assuming you do not want to pass a tuple as notify_email parameter so you are probably missing a [0] after the fetchone() when reading the admin_email.

Caveat: I am not a python programmer. And haven't tried the program below. Also the refactored code still needs some Inversion of control refactoring, but ignore this if you do not already know what that means from other languages.

def get_option(cnx, blog_id, option_name):
    table_name = 'wp_' + str(blog_id) + '_options'
    query_ = "SELECT option_value FROM " + table_name + " WHERE option_name=%s"
    with closing( cnx.cursor() ) as cursor_:
        cursor_.execute(query_, (option_name,))
        return cursor_.fetchone()[0]

def get_timespan():
    date_to = datetime.datetime.now()
    # if it monday lets get activity since saturday
    if date_to.weekday() is 0:
        date_from = date_to - datetime.timedelta(days=2)
    # else just get the activity from the past day
    else:
        date_from = date_to - datetime.timedelta(days=1)
    return (date_from, date_to)

def export_blog_activity(blog_id, cnx, mandrill_client):
    notify_email = get_option(cnx, blog_id, 'admin_email')
    encoded_result = get_option(cnx, blog_id, 'wysija')
    decoded_result = phpserialize.unserialize(encoded_result.decode('base64', 'strict'))
    sender_email = decoded_result['from_email']
    date_from, date_to = get_timespan()
    mandrill_client.exports.activity(notify_email, date_from, date_to, None, sender_email,'Sent')

def main():
    # database name and credentials
    # these should eventually be parsed from command line arguments
    database_config = {
        'user': 'root',
        'password': 'password',
        'database': 'wpmudb'
    }
    # Mandrill API Key
    # this should also be parsed from command line
    mandrill_api_key = 'xxxxx_xxxxxxxxxxxxxxxx'
    mandrill_client = mandrill.Mandrill(mandrill_api_key)
    with closing(mysql.connector.connect(**database_config)) as cnx:
        export_activity = functools.partial(export_blog_activity, mandrill_client = mandrill_client, cnx=cnx)
        with closing( cnx.cursor() ) as cursor_:
            # Get all blogs but blog_id 1 since the main site needs no report
            query = "SELECT blog_id FROM wp_blogs WHERE blog_id != '1'"
            cursor_.execute(query)
            for row in cursor_:
                export_activity(row[0])

if __name__ == "__main__":
    try:
        main()
    except mysql.connector.Error as e:
        print "Sql error:", e