See the first post in The Pragmatic Programmer 20th Anniversary Edition series for an introduction.
Challenge 1
Help strengthen your team by surveying your project neighborhood. Choose two or three broken windows and discuss with your colleagues what the problems are and what could be done to fix them.
I’ll discuss one example from work recently - exact details removed of course. Our current main project is the next version (3) of our main API. Since it’s a new version we’re able to make breaking changes, however there is still a fair amount backwards compatibility required. The new API had to translate between the domain model of existing users in the previous API version and the newer model. The previous model had a number of user types, each with different permissions.Here’s a few examples (there were more):
- Read Only
- Standard
- Super
These were represented using a user_type
field with values such as:
READONLY = "readonly"
STANDARD = "standard"
SUPER = "super"
The new Django User
model also has a user_type
field which maps to this field. For reasons I won’t get into however, the new user_type
s have a _user
suffix. The implementation of this using choices was along the lines of:
from django.db import models
class User(models.Model):
# user_type values used in API V2
READONLY = "readonly"
STANDARD = "standard"
SUPER = "super"
# mapping V2 user_type to Django group name
USER_TYPES = [
(READONLY, "readonly_user"),
(STANDARD, "standard_user"),
(SUPER, "super_user"),
]
user_type = models.CharField(choices=USER_TYPES)
Ignoring the many missing details from this example; the code works. The previous user_type
s are mapped to the new ones and the values for the new user_type
are constrained as desired using Django choices
. It was also implemented in a short period of time - great right? Well, there are some broken windows:
- The string values for the V2 API
user_type
are class attributes on the newUser
model. This clutters the interface of theUser
model and their purpose can only be known by reading the code and accompanying comments. - The
USER_TYPES
list used for thechoices
argument in theuser_type
field is very un-DRY (wet?).
I brought these up during code review and we fixed the broken windows before the pull request was accepted:
from enum import Enum
from django.db import models
class V2UserTypes(Enum):
READONLY = "readonly"
STANDARD = "standard"
SUPER = "super"
class User(models.Model):
user_type = models.CharField(
choices=[
(t, t.value + "_user") for t in V2UserTypes
]
)
The interface clutter for the User
model was removed by moving the version 2 string constants out of the class and removing the USER_TYPES
list (it’s only purpose was to support the choices
parameter of the user_type
field). The string constants are now encapsulated within an Enum
. This has two main benefits:
- It is very clear what the values of
V2UserTypes
represent - The
Enum
ensures unique, constant values. Helping to protect them against the double edged sword of a high level of dynamicism offered by Python.
A list comprehension was introduced to add the _user
suffix the the version 2 user types to keep the code DRY.
Since this is a new project, fixing this broken window before it got into the codebase was important to ensure that the codebase remained high quality.
Challenge 2
Can you tell when a window first gets broken? What is your reaction? If it was the result of someone else’s decision, or a management edict, what can you do about it?
Challenge 1 was an example of being able to spot when a window first gets broken. My reaction in this case was to bring it up during code review; explaining why I thought it was a broken window, the issues it could cause in the future and providing some potential solutions. This was a result of someone else’s decision, however I am fortunate to be on a team with which encourages code review and feedback/learning from others in general.
A management edict, on the other hand, would be more difficult to address. Of course it depends on exactly what the edict was however my general approach would be:
- Extract the broken window from the edict
- Gather evidence from previous projects, open source, research papers etc where a similar broken window was introduced
- Explain why the broken window is an issue and (using the evidence) what the consequences could be
- Provide a solution, most likely something along the lines of “I would like x days to implement x solution. This additional time will save y amount of issues in the future <reference evidence here>”