This post originally appeared on steadbytes.com
See the first post in The Pragmatic Programmer 20th Anniversary Edition series for an introduction.
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
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_types 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_types 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_typeare class attributes on the new
Usermodel. This clutters the interface of the
Usermodel and their purpose can only be known by reading the code and accompanying comments.
USER_TYPESlist used for the
choicesargument in the
user_typefield 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
Enumensures 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.
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>"