Program for sending emails

Question

Disclaimer: This question is very much like the one posted here. I gathered some opinions about my options from the answer there. Here, I just want validation about the choices I'm deciding to stick to and see what people think about the decisions specifically. Also, gather suggestions about other parts of the code I'm not specifically asking about, since I'm new to Python OOP.

Scenario: I'm writing a program that will send emails. For an email, the to, from, text and subject fields will be required and other fields like cc and bcc will be optional. Also, there will be a bunch of classes that will implement the core mail functionality, so they will derive from a base class (Mailer).

Following is my incomplete code snippet:

class Mailer(object):
    __metaclass__ == abc.ABCMeta

    def __init__(self,key):
        self.key = key

    @abc.abstractmethod
    def send_email(self, mailReq):
        pass


class MailGunMailer(Mailer):
    def __init__(self,key):
        super(MailGunMailer, self).__init__(key)

    def send_email(self, mailReq):
        from = mailReq.from
        to = mailReq.to
        subject= mailReq.subject
        text = mailReq.text
        options = getattr(mailReq,'options',None)
        if(options != None):
            if MailRequestOptions.BCC in options:
                #use this property
                pass
            if MailRequestOptions.CC in options:
                #use this property
                pass



class MailRequest():
    def __init__(self,from,to,subject,text):
        self.from = from
        self.to = to
        self.subject = subject
        self.text = text

    def set_options(self,options):
        self.options = options

class MailRequestOptions():
    BCC = "bcc"
    CC = "cc"

I've made the following decisions about code designs. What do you think about them?

1. The send_email() method will take four required parameters - to, from, subject and text, and bunch of other optional parameters like cc, bcc etc. So I decided to create the MailRequest wrapper, which will take the 4 required fields in the constructor, and the other optional parameters will go in the options dict.

   Is this an acceptable way of doing this? Why not use **kwargs?

   Because if you define something like:

   def foo(**kwargs):
       pass
   

   Then to call it, you can't do something like:

   options = []
   options["one"] = 1
   options["two"] = 2
   foo(options)
   

   Right?

   You'd have to do something like:

   foo(one=1,two=2)
   

   Now if I have 15 parameters that foo accepts, then **kwargs isn't a good way to do this right?

2. I've created the MailRequestOptions class to contain static strings. The reason behind the existence of this class is, even if the user knows he has to pass some options in the options dict of the MailRequest object, how would he know which options can he set. This class could probably help the user know about what options can be set. This will also be helpful if the user has auto complete in an IDE or something. Do you think I'm thinking right? Or is this a somewhat unusual way of doing things?

Answer 1

First thing first - PEP 8 is the de-facto style guide for Python. It's a good idea to just stick to it, and for the most part everyone else will too.

This would look like

class Mailer(object):
    __metaclass__ == abc.ABCMeta

    def __init__(self, key):
        self.key = key

    @abc.abstractmethod
    def send_email(self, mail_req):
        pass


class MailGunMailer(Mailer):
    def __init__(self, key):
        super(MailGunMailer, self).__init__(key)

    def send_email(self, mail_req):
        from_ = mail_req.from_
        to = mail_req.to
        subject= mail_req.subject
        text = mail_req.text
        options = getattr(mail_req, 'options', None)

        if options is not None:
            if MailRequestOptions.bcc in options:
                # use this property
                pass
            if MailRequestOptions.cc in options:
                # use this property
                pass


class MailRequest:
    def __init__(self, from_, to, subject, text):
        self.from_ = from_
        self.to = to
        self.subject = subject
        self.text = text

    def set_options(self,options):
        self.options = options


class MailRequestOptions:
    bcc = "bcc"
    cc = "cc"

There are a couple of logic changes (eg. is not None instead of != None), but nothing significant.

Next thing I notice is

options = getattr(mail_req, 'options', None)

This is an absolute red flag to me. Attributes should not exist sometimes and not other times. Only in odd cases is such a thing appropriate. This is easy enough to fix with

self.options = None  # = {}?

in MailRequest.__init__.

Your set_options method is pointless - MailRequest.options is public. Since you intend to use it as a struct-like container, keep it that way.

I'm not sure why you even have the options parameter though; your options have a static set of accepted attributes so just inline it. Again, this removes the use of existence as a variable. Attribute existence is a terrible way of hiding state.

class MailRequest:
    def __init__(self, from_, to, subject, text):
        self.from_ = from_
        self.to = to
        self.subject = subject
        self.text = text

        self.bcc = None
        self.cc = None

This also removes the very strange MailRequestOptions class.

Then I look at the Mailer ABC. Remember that Python is duck-typed. The reason for an ABC is to standardize common interfaces that arise in the code - unless you have at least a few implementations, duck-typing is more appropriate.

Which gives

class MailGunMailer:
    def __init__(self, key):
        self.key = key

    def send_email(self, mail_req):
        from_ = mail_req.from_
        to = mail_req.to
        subject= mail_req.subject
        text = mail_req.text

        if mail_req.bcc is not None:
            # use this property
            pass

        if mail_req.cc is not None:
            # use this property
            pass


class MailRequest:
    def __init__(self, from_, to, subject, text):
        self.from_ = from_
        self.to = to
        self.subject = subject
        self.text = text

        self.bcc = None
        self.cc = None

Fundamentally this is also inappropriate. Your first question gives reasons that normal arguments might not be appropriate, but Nizam Mohamed points out some flaws in your understanding. In fact, having a lot of parameters in a signature is fine. Objects to hold options exist for several reasons:

• Some languages have little flexibility in argument passing and little ability to name arguments
• Some languages don't let you pack up arguments and pass them around easily
• There are cases where the arguments form an object that gets reused

The first two points are not good reasons in Python. See subprocess for an example of something that just offers a large number of arguments. The caller does not have to specify them all since most are default arguments.

The last option does happen, but it is more akin to typical OO abstractions. And as usual with abstractions, premature abstraction is bad.

In this case (in isolation), just offer the arguments directly.

class MailGunMailer:
    def __init__(self, key):
        self.key = key

    def send_email(self, from_, to, subject, text, bcc=None, cc=None):
        if bcc is not None:
            # use this property
            pass

        if cc is not None:
            # use this property
            pass

In response to your second question, you are adding dynamicism where naturally there is none. This doesn't help. If you want to document the options, do it the one obvious way: in the options. (This is what inlining options achieved.) Simple, see?

>>> import this

The Zen of Python, by Tim Peters

Beautiful is better than ugly.
Explicit is better than implicit.
Simple is better than complex.
Complex is better than complicated.
Flat is better than nested.
Sparse is better than dense.
Readability counts.
Special cases aren't special enough to break the rules.
Although practicality beats purity.
Errors should never pass silently.
Unless explicitly silenced.
In the face of ambiguity, refuse the temptation to guess.
There should be one-- and preferably only one --obvious way to do it.
Although that way may not be obvious at first unless you're Dutch.
Now is better than never.
Although never is often better than *right* now.
If the implementation is hard to explain, it's a bad idea.
If the implementation is easy to explain, it may be a good idea.
Namespaces are one honking great idea -- let's do more of those!

Answer 2

Because if you define something like:

def foo(**kwargs):
    pass

Then to call it, you can't do something like:

options = []
options["one"] = 1
options["two"] = 2
foo(options)

Right?

The aswer is Yes. But there are ways,

**name is used in two ways.

To define optional keyword argumnets in function definiton.

def f(**kwargs): pass. Here the argument of function f, kwargs is optional. You can legitimately call f like f() i.e without arguments. It's utility is you can pass unlimited number of keyword arguments like one=1,...

To unpack a dict to pass arguments at function invocation.

If a function is defined with some arguments whether it's required or optional, its arguments can be passed by **dict when invoking the function. f.e

def f(a,b=2): pass

This function has two arguments one is required. To call it with one argument, you can do,

arg = dict(a=111)
f(**arg)

With two argumnets,

arg = dict(a=111,b=222)
f(**arg)

Combining the two **name form.

def f(**kwargs): pass  

can be called as follows,

f()
f(a=1,b=2,c=3)
arg = dict(foo=1,bar=2)
f(**arg)

The send_email method can be simplified.

def send_email(self, from, to, sub, body, **options):
    ''' 
        This method sends an email by taking four req. args
        from, to, sub, body and optional 'cc' and 'bcc' keyword args
    ''' 
    from = from
    to = to
    subject= sub
    text = body
    cc = options.get('cc')
    if cc is not None:
            #use this property
    bcc = options.get('bcc')
    if bcc is not None:
            #use this property

The 'docstring' can be checked by calling,

help('classname or obj'.send_email)
'classname or obj'.send_email.__doc__

and can be called as follows,

obj.send_email('me@example.com', 'you@example.com', 'hi', 'howdy', cc='he@example.com')  

or

mail1 = dict(from='she@example.com', to='he@example.com', sub='date', body='free tonight?', bcc='you@example.com')
obj.send_email(**mail1)