Every description I have read about god classes considers them to be an anti-pattern and a bad practice. Most descriptions I have read about mixins considers them to be acceptable in some cases. If the functionality of a god class is broken up into several mixins, each mixin can be separately unit tested and separation of concerns is still maintained, at least to some degree. When it comes down to it, I'd still like to encapsulate all of the functionality into one object. I'd like to get opinions on this design. Is this design a bad practice?
Is Using a God Class with Mixins Still Bad Practice?
anti-patternsmixinspython
Related Solutions
Having written and then refactored a python "God object", I sympathize. What I did is break the original object down into sub sections based upon methods. For instance, the original looked like this pseudo code:
method A():
self.bla += 1
method B():
self.bla += 1
do stuff():
self.bla = 1
method A()
method B()
print self.bla
The stuff method is a self contained "unit" of work. I migrated it out to a new class which the original instantiates. This pulled out the necessary properties as well. Some were used only by the sub class and could move straight across. Others were shared, and got moved into a shared class.
The "God object" creates a new copy of the shared class at start up, and each of the new sub classes accepts a pointer as part of their init method. For example, here's a stripped version of the mailer:
#!/usr/bin/env python
# -*- coding: ascii -*-
'''Functions for emailing with dirMon.'''
from email.MIMEMultipart import MIMEMultipart
from email.MIMEBase import MIMEBase
from email.MIMEText import MIMEText
from email.Utils import COMMASPACE, formatdate
from email import Encoders
import os
import smtplib
import datetime
import logging
class mailer:
def __init__(self,SERVER="mail.server.com",FROM="support@server.com"):
self.server = SERVER
self.send_from = FROM
self.logger = logging.getLogger('dirMon.mailer')
def send_mail(self, send_to, subject, text, files=[]):
assert type(send_to)==list
assert type(files)==list
if self.logger.isEnabledFor(logging.DEBUG):
self.logger.debug(' '.join(("Sending email to:",' '.join(send_to))))
self.logger.debug(' '.join(("Subject:",subject)))
self.logger.debug(' '.join(("Text:",text)))
self.logger.debug(' '.join(("Files:",' '.join(files))))
msg = MIMEMultipart()
msg['From'] = self.send_from
msg['To'] = COMMASPACE.join(send_to)
msg['Date'] = formatdate(localtime=True)
msg['Subject'] = subject
msg.attach( MIMEText(text) )
for f in files:
part = MIMEBase('application', "octet-stream")
part.set_payload( open(f,"rb").read() )
Encoders.encode_base64(part)
part.add_header('Content-Disposition', 'attachment; filename="%s"' % os.path.basename(f))
msg.attach(part)
smtp = smtplib.SMTP(self.server)
mydict = smtp.sendmail(self.send_from, send_to, msg.as_string())
if self.logger.isEnabledFor(logging.DEBUG):
self.logger.debug("Email Successfully Sent!")
smtp.close()
return mydict
It is created once and shared between the different classes that need mailing capabilities.
So for you, create a class larry
with the properties and methods you need. Everywhere the client says larry = blah
replace it with larryObj.larry = blah
. This migrates things to sub projects without breaking the current interface.
The only other thing to do is look for "units of work". If you were going to turn part of the "God Object" into it's own method, do so. But, put the method outside it. This forces you to create an interface between the components.
Laying that groundwork allows everything else to follow it. For example, a piece of the helper object demonstrating how it interfaces with the mailer:
#!/usr/bin/env python
'''This module holds a class to spawn various subprocesses'''
import logging, os, subprocess, time, dateAdditionLib, datetime, re
class spawner:
def __init__(self, mailer):
self.logger = logging.getLogger('dirMon.spawner')
self.myMailer = mailer
Concentrate on the smallest individual unit of work possible, and move it out. This is easier to do, and lets you play with the setup quickly. Don't look at properties for moving stuff, they are ancillary to the tasks being done with them in most cases. Whatever is left over after you have dealt with the methods should probably stay in the original object, since it is part of the shared state.
But, the new objects should now accept the properties they need as init variables, not touching the caller objects property. They then return any necessary values, which the caller can use to update the shared properties as necessary. This helps to decouple the objects and makes for a more robust system.
Yes, these are mixins. They aren't as powerful as Scala traits (no ability to have a reference to the parent "self" object and its properties or methods), but the default methods can implement meaningful behaviour and so can be more more than sticking plasters to stop existing code breaking when an interface is extended.
The limited power of default methods seems to be deliberate, judging by the various informed discussions of the feature. The designers' advice is that default methods should generally not contain the concrete implementation of a method but accept a lambda or reference to a concrete class, either of which can provide the implementation. Example here
Best Answer
Sorry to be the bearer of bad news:
Yes.
The larger your application gets, the more trouble this will cause. What happens when you try to name a method
do_thing
, import it into your god class, and find that it already has a method nameddo_thing
? Now you have to worry about namespace collisions in your god class: what a pain!Another (small) one: if everything goes here, and multiple people are working on different modules, but everyone has to "register" things in your god class, then you will be dealing with lots of unnecessary merge conflicts for that class in your code repository. If separate things were always separate, then you have less to worry about from people stepping on eachother's toes.
These are just a few things that come to mind. Really though, you are approaching it from the wrong perspective: why would you want to do this? If you have everything nicely separated and organized, why would you want to undo all of that and throw it all into a single class? Doing this might save you a few
import
statements at the top of your code, or a few less dependencies that need to be injected, but none of that is substantially improving your code base, and it comes at a real cost. It's a simple formula:cost + no benefit = bad idea
. You're already doing the "hard" part of dividing your application nicely. Don't mess that up on the home stretch.