I have a big class with too many methods. How do I split it?
So there you are, with a class that has over 100 (200? 500? 1000?) lines of code in Python. Wow, such an impressive achievement. Life with a monster like this can’t be easy.
History of our giant class is irrelevant. What's important is that our patient is sick and needs a treatment. We'll cure it, but making a proper diagnosis first is crucial. Then we'll apply appropriate therapy called refactoring. Hopefully lessons learned this way will save you some nerves in the future and help avoiding mistakes.
Before we take a look at our patient, let me start with telling you what this code is supposed to do. We are building a commercial VOD platform. Access to videos is paid, so we have to implement payments module support. A 3rd party service for payment gateway has been chosen - Respectable (naturally, I made up this one). We still have to write a client code, because Respectable does not provide any SDK. At least it has a REST API. To make this example a bit simpler, let's say we need only two of its methods - sale for charging customer's payment card and refund for giving their money back.
We hand these requirements over to the dev team and after a few days we get an implementation. Please read it carefully to better understand next steps I made:
import urllib.parse
import requests
from django.conf import settings
from requests.auth import HTTPBasicAuth
from requests.exceptions import RequestException
logger = logging.getLogger(__name__)
class RespectableApiException(Exception):
pass
class RespectableApi:
BASE_URL = settings.RESPECTABLE_API_BASE_URL
SALE_ACTION = 'sales'
REFUND_ACTION = 'refunds'
def sale(self, payment_card_details, amount):
params = self._get_sale_params(payment_card_details, amount)
result = self._run_query(params)
return self._prase_sale_response(result)
def refund(self, payment_card_details, amount):
params = self._get_refund_params(payment_card_details, amount)
result = self._run_query(params)
return self._parse_refund_response(result)
def _get_sale_params(self, payment_card_details, amount):
return {
'action': self.SALE_ACTION,
'pan': payment_card_details.card_number,
'cvc': payment_card_details.cvc,
'exp_month': payment_card_details.expiration_date.month,
'exp_day': payment_card_details.expiration_date.day,
'amount': amount
}
def _get_refund_params(self, payment_card_details, amount):
return {
'action': self.REFUND_ACTION,
'pan': payment_card_details.card_number,
'amount': amount
}
def _parse_refund_response(self, result):
if 'success' not in result or 'refund_amount' not in result:
raise RespectableApiException('Malformed refund response: %r', result)
if result['success'] and result['refund_amount'] > 0:
return True
else:
return False
def _parse_sale_response(self, result):
if 'success' not in result:
raise RespectableApiException('Malformed sale response: %r', result)
return bool(result['success'])
def _get_auth_data(self):
return HTTPBasicAuth(
settings.RESPECTABLE_API_USERNAME,
settings.RESPECTABLE_API_PASSWORD
)
def _run_query(self, params):
url = urllib.parse.urljoin(self.BASE_URL, params['action'])
del params['action']
auth = self._get_auth_data()
logger.info('Sending request to %s with amount %s', url, params['amount'])
try:
response = requests.post(url, json=params, auth=auth)
except RequestException as e:
logger.info(
'Request to %s with amount %s failed with exception',
url, params['amount'], e
)
raise RespectableApiException('Request failed')
logger.info(
'Sent request to %s with amount %s, response %r',
url, params['amount'], response
)
decoded_response = response.json()
if response.status_code == 200:
return decoded_response
else:
try:
error_message = decoded_response['error']
except KeyError:
error_message = 'Request failed with status {}'.format(response.status_code)
raise RespectableApiException(error_message)
There are few different problems with this class, but let's start with the most noticeable one - its size. RespectableApi is so big because it combines several responsibilites - communication with external API through requests library, parsing responses, handling errors. It exposes two public methods - sale and refund. The rest of code in this class is just for these methods, which are very similar by the way.
Mixins!
Let's try splitting the class. You might think of an approach with mixins. First, we take all the code that is used by both sale and refund and create some fuzzy Base class:
class BaseRespectableApi:
BASE_URL = settings.RESPECTABLE_API_BASE_URL
def _get_auth_data(self):
return HTTPBasicAuth(
settings.RESPECTABLE_API_USERNAME,
settings.RESPECTABLE_API_PASSWORD
)
def _run_query(self, params):
url = urllib.parse.urljoin(self.BASE_URL, params['action'])
del params['action']
auth = self._get_auth_data()
logger.info('Sending request to %s with amount %s', url, params['amount'])
try:
response = requests.post(url, json=params, auth=auth)
except RequestException as e:
logger.info(
'Request to %s with amount %s failed with exception',
url, params['amount'], e
)
raise RespectableApiException('Request failed')
logger.info(
'Sent request to %s with amount %s, response %r',
url, params['amount'], response
)
decoded_response = response.json()
if response.status_code == 200:
return decoded_response
else:
try:
error_message = decoded_response['error']
except KeyError:
error_message = 'Request failed with status {}'.format(response.status_code)
raise RespectableApiException(error_message)Then we take remaining bits of code and create two smaller classes (mixins) for each API method:
class RefundRespectableApiMethodMixin:
REFUND_ACTION = 'refunds'
def refund(self, payment_card_details, amount):
params = self._get_refund_params(payment_card_details, amount)
result = self._run_query(params)
return self._parse_refund_response(result)
def _get_refund_params(self, payment_card_details, amount):
return {
'action': self.REFUND_ACTION,
'pan': payment_card_details.card_number,
'amount': amount
}
def _parse_refund_response(self, result):
if 'success' not in result or 'refund_amount' not in result:
raise RespectableApiException('Malformed refund response: %r', result)
if result['success'] and result['refund_amount'] > 0:
return True
else:
return False
class SaleRespectableApiMethodMixin:
SALE_ACTION = 'sales'
def sale(self, payment_card_details, amount):
params = self._get_sale_params(payment_card_details, amount)
result = self._run_query(params)
return self._prase_sale_response(result)
def _get_sale_params(self, payment_card_details, amount):
return {
'action': self.SALE_ACTION,
'pan': payment_card_details.card_number,
'cvc': payment_card_details.cvc,
'exp_month': payment_card_details.expiration_date.month,
'exp_day': payment_card_details.expiration_date.day,
'amount': amount
}
def _parse_sale_response(self, result):
if 'success' not in result:
raise RespectableApiException('Malformed sale response: %r', result)
return bool(result['success'])And finally we tie all three together to be able to use it just like we did before:
class RespectableApi(BaseRespectableApi, RefundRespectableApiMethodMixin, SaleRespectableApiMethodMixin):
passSo now we have four significantly smaller classes with a bit of code separation. All sale specific code now belongs to SaleRespectableApiMethodMixin class. So is true with refund and its RefundRespectableApiMethodMixin.
Is that a good code we can live with?
There is a designated area in hell for people creating such abominations. It does not really help with coupling between API methods and transport mechanism which is REST in this case. Imagine how it would look like to add support for more API methods. A nightmare.
In fact, reading this code to fully understand what RespectableApi is requires a lot of jumping through all classes involved. They will be probably scattered through few files. Having them in just one .py file makes situation even worse. Cognition load is not reduced - it is increased by extra classes. At least changing anything in sale specific code won't break refunds and vice versa.
Usually this approach is the worst idea you might have.
Inheritance and standalone API commands classes
A slightly better approach is to use BaseRespectableApi as a base class for our API methods, so we'll get:
class RefundRespectableApiMethod(BaseRespectableApi):
REFUND_ACTION = 'refunds'
def refund(self, payment_card_details, amount):
params = self._get_refund_params(payment_card_details, amount)
result = self._run_query(params)
return self._parse_refund_response(result)
def _get_refund_params(self, payment_card_details, amount):
return {
'action': self.REFUND_ACTION,
'pan': payment_card_details.card_number,
'amount': amount
}
def _parse_refund_response(self, result):
if 'success' not in result or 'refund_amount' not in result:
raise RespectableApiException('Malformed refund response: %r', result)
if result['success'] and result['refund_amount'] > 0:
return True
else:
return False
class SaleRespectableApiMethod(BaseRespectableApi):
SALE_ACTION = 'sales'
def sale(self, payment_card_details, amount):
params = self._get_sale_params(payment_card_details, amount)
result = self._run_query(params)
return self._prase_sale_response(result)
def _get_sale_params(self, payment_card_details, amount):
return {
'action': self.SALE_ACTION,
'pan': payment_card_details.card_number,
'cvc': payment_card_details.cvc,
'exp_month': payment_card_details.expiration_date.month,
'exp_day': payment_card_details.expiration_date.day,
'amount': amount
}
def _parse_sale_response(self, result):
if 'success' not in result:
raise RespectableApiException('Malformed sale response: %r', result)
return bool(result['success'])The good part is that clients of Respectable API still have to use just one class (SaleRespectableApiMethod or RefundRespectableApiMethod) to achieve what they need. Code of sales is completely separated from refunds. But again, this solution does not reduce complexity of individualy API method. It still relies on a base class and we have to read it to fully understand what a single ApiMethod class does when it's invoked. Inheritance introduces a very rigid dependency between classes.
Separation by concerns
Wait, there must be a better way. There is one. Let's split code by concerns it should care about.
RespectableAPI - main class that will handle REST communication and handling errors. We could also move transport details to another class, but this could obfuscate this example (and obviously there is no visible value in this since this is the only transport mechanism Respectable handles). It will have one public method execute, that will accept...
RespectableAPIMethod - abstract base class for all API methods. It does not use (neither know anything about) RespectableAPI. The latter uses methods defined in base class to prepare params and parse response.
SaleRespectableAPIMethod, RefundRespectableAPIMethod - concrete classes inheriting from RespectableAPIMethod.
Let's take a look at code:
class RespectableApi:
BASE_URL = settings.RESPECTABLE_API_BASE_URL
def execute(api_method):
params = api_method.get_params()
result = self._run_query(params)
return api_method.parse_response(result)
def _get_auth_data(self):
... # irrelevant, no changes
def _run_query(self, params):
... # irrelevant, no changesAs you can see, execute method argument should be an object with two methods: get_params and parse_response. Base class for API methods looks like this:
class BaseRespectableApiMethod(metaclass=abc.ABCMeta):
@abc.abstractmethod
def get_params(self):
pass
@abc.abstractmethod
def parse_response(self):
passImplementation of refund (I omit sale, because it won't contribute much at this stage):
class RefundRespectableApiMethod(BaseRespectableApiMethod):
REFUND_ACTION = 'refunds'
def __init__(self, payment_card_details, amount):
self.payment_card_details = payment_card_details
self.amount = amount
def get_params(self, payment_card_details, amount):
return {
'action': self.REFUND_ACTION,
'pan': payment_card_details.card_number,
'amount': amount
}
def parse_response(self, result):
if 'success' not in result or 'refund_amount' not in result:
raise RespectableApiException('Malformed refund response: %r', result)
if result['success'] and result['refund_amount'] > 0:
return True
else:
return FalseNotice that we no longer need to pass extra arguments to execute method, because they are saved in RefundRespectableApiMethod instance.
To me, this is the most elegant solution of all three presented in this article. API methods classes are small and can be very easily unit tested. Our original class, RespectableApi, is now simpler.
Adding new API methods should not be any pain in the neck.
The downside is that now client has to use two classes to achieve what they want instead of one. We can still be nice though if we hide this little inconvenience using Facade design pattern:
class RespectableApiFacade:
def sale(payment_details, amount):
api_method = SaleRespectableApiMethod(payment_details, amount)
api_gateway = RespectableApi()
return api_gateway.execute(api_method)
def refund(payment_details, amount):
api_method = RefundRespectableApiMethod(payment_details, amount)
api_gateway = RespectableApi()
return api_gateway.execute(api_method)
That's all folks. Looking forward to hearing from you about other patterns/approaches to decoupling code.
DISCLAIMER: Code in this post was hardly tested, because it was not taken from a piece of working software. My intention was to present an idea, not to provide working examples.
Comments powered by Disqus.