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?

• Legato: Welcome to CodeReview. Here we review complete code. If your code is incomplete and/or not fully functional it will be closed as off-topic.

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!

• GrowinMan: Wow. Thanks a lot for taking the time for the detailed reply. I really appreciate it. BTW, here is something I didn't mention before: This was only a partial implementation. I wanted some code review before I went ahead and completed the project. I wasn't aware we're only supposed to post completed code on this site, apologies for that. I'm definitely going to add more implementations of the Mailer ABC so I guess I'll keep that
• GrowinMan: So by 'If you want to document the options, do it the one obvious way: in the options. ' you just mean use **options or specify arguments the way you have in the final suggested code, and just use the docstring, right?
• Veedrac: @GrowinMan Hopefully I've explained why writing out the arguments in full is better (**kwargs is mostly for forwarding arguments), but either way one should document. Treat documentation as mandatory ;).
• Veedrac: @GrowinMan Wrt. partial code - it's fine to post a subset of code, but people are then also allowed to review that code as given. As long as the code is real, it's fine. I tried to give justifications so it's clear what advice does apply to the code in full.

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('[email protected]', '[email protected]', 'hi', 'howdy', cc='[email protected]') 

or

mail1 = dict(from='[email protected]', to='[email protected]', sub='date', body='free tonight?', bcc='[email protected]')
obj.send_email(**mail1)