Somewhere between under- and overengineering

At the beginning there were requirements

Imagine that you are creating an application which has, among others, the following requirements:

1. There should be a user profile containing the following information:
2.     Email,
3.     Status,
5.     Last status change.
8. The email should be read only.
9. Status can be either “active” or “inactive”.
10. Users should be notified when their status changes.

Take 1: the simplistic approach

The first model that would handle these requirements would be something like:

from datetime import datetime

class User:
email: str
status: str
last_status_change: datetime

def notify_about_status_change(self):
print(f'Sending notification to {self.email}')

Is it a good code? Definitely not.

1. Email can be changed and does not have to be set when a User instance is created.
2. The status choice is not limited to a known set of values (what if in one place the programmer makes a typo)?
3. The software developer needs to remember to update last_status_change whenever status is changed.

Take 2: setter/getter approach

Can we do better? Of course.

from datetime import datetime
from enum import Enum


class Status(Enum):
ACTIVE = 'ACTIVE'
INACTIVE = 'INACTIVE'


class User:
_email: str
_status: str
_last_status_change: Status

def __init__(self, email: str):
self._email = email
self._status = Status.ACTIVE
self._last_status_change = datetime.now()

@property
def email(self) -> str:
return self._email

@property
def status(self) -> Status:
return self._status

@status.setter
def status(self, status: Status):
self._status = status
self._last_status_change = datetime.now()
self._notify_about_status_change()

@property
def last_status_change(self) -> datetime:
return self._last_status_change

def _notify_about_status_change(self):
print(f'Sending notification to {self._email}')

Of course, there are problems like the one that we still get to the protected fields and modify them or that we can assign something that is a string to status instead of proper enumerated value, but let’s try not to correct the language drawbacks.

The code has improved:

1. Email and last status change are read only,
2. Status can be chosen from a known set of values,
3. last_status_change is updated automatically, so the programmer does not need to think about it.

Cool. But, once again: can we do better? Sure. Look at the status. The encapsulation here is superficial because in fact, it is just more code to do what we would do without setters and getters (apart from automatically updating the timestamp). What we really want is the ability to activate and deactivate the user. We should not know how it is implemented internally. If we wanted to change the underlying mechanism, we would need to touch multiple places of our project. This violates the open-closed principle because adding new status might require us to modify existing code instead of writing new functionality.

The next issue are the side effects. When I set status I expect status to be changed and not email to be sent! There are too many side effects. Moreover, if in the future we wanted to expand the capability of last status change updates by adding timezone support can cause trouble because a programmer will be tempted to handle this task in the status setter, while the change is only connected to the timestamp.

Last, but not least, sending notifications is not a behavior of the user. The user might want to log in, send a message to someone else, but not notify himself that their status has been changed.

Take 3: more clever encapsulation

So, let’s refactor once again.

from datetime import datetime
from enum import Enum


class Status(Enum):
ACTIVE = 'ACTIVE'
INACTIVE = 'INACTIVE'


class User:
_email: str
_status: str
_last_status_change: Status

def __init__(self, email: str):
self._email = email
self._status = Status.ACTIVE
self._update_last_status_change()

@property
def email(self) -> str:
return self._email

@property
def status(self) -> Status:
return self._status

def activate(self):
self._status = Status.ACTIVE
self._update_last_status_change()

def deactivate(self):
self._status = Status.INACTIVE
self._update_last_status_change()

def _update_last_status_change(self):
self._last_status_change = datetime.now()

@property
def last_status_change(self) -> datetime:
return self._last_status_change

def notify_about_status_change(user: User):
print(f'Sending notification to {user.email}')

Cleaner encapsulation has been achieved, no unexpected side effects take place, notifications are sent by the system and not by the user, status change timestamps can now be more easily extended with new features, we don’t know how are the statuses stored… Or do we? Of course we do. We’ve only changed the status setter to activate/deactivate methods, but while retrieving it, we still know that Status class is used.

Take 4: overengineering

We need to introduce just this tiny change…

from datetime import datetime
from enum import Enum


class Status(Enum):
ACTIVE = 'ACTIVE'
INACTIVE = 'INACTIVE'


class User:
_email: str
_status: str
_last_status_change: Status
# ...
@property
def is_active(self) -> bool:
return self._status == Status.ACTIVE

@property
def is_inactive(self) -> bool:
return self._status == Status.INACTIVE

Let’s stop here before we rush once again to search for potential pitfalls and improve our code. Think for a moment: is it worth it? At which stage will this code be good enough? Does this is_active property give me better control over my code or am I doing it only to feel better?

Both scruffiness and perfectionism are the roots of all evil

I am not saying that we should leave the code as in the first example. But keep one thing in mind: guidelines and rules of thumb exist for programming and not the other way round. It doesn’t really matter if you decide to loosen the principles if you know what you are doing. The design patterns are extremely useful, but putting them everywhere you see an opportunity may lead to overhead so big that the benefits of using clean solutions will be lost.

It’s good to know what makes code maintainable and clean and what makes it a disaster to work with, but the stiff principles are not everything. Common sense should also play a big role. And apart from being able to notice where the open-closed principle has been violated or where composition should be used instead of inheritance, a good programmer should also be able to foresee the negative impact of perfectionism. This takes time and experience and there are never good answers to those issues.

One could argue that my code should either be improved further or I should have left it as it is one step earlier and good arguments in favor of both cases can be found. The goal of this article was to make you feel uncomfortable twice: first time with the first poorly thought through example and second time when I started to care more about rules and less about what was the original task. The question that should be asked is not “do I use all good practices” neither “how to write less code”, but “from what I’ll benefit most?”