How misnaming a class hurt us royally.
Naming things properly is important. Since human brain can hold limited information at one time, it is important to minimize the difference between what an entity is called and its functionality. A popular mind game has the words GREEN, RED, and YELLOW, written in red, yellow and green respectively, and the challenge is to speak out the colors (and not the words) as fast as you can.
Choosing meaningful name is actually not that hard. Of course number of items will
be denoted by nItems, and total height will be denoted by
totalHeight. A function to compute the sum will be called
computeSum() and a function to check for alerts will be called as
checkForAlerts.
What is hard is that the names continue to be meaningful. It is hard for the names to keep
up with change in the functionality of the underlying entity. Let's say you
have a function which sends messages.
You begin by call it sendMessages(). Do you update the name when the function
starts to update some stats? Do you update the name when the function modifies
the message a little before sending to the recipients? How about the scenario when
the function validates the message before sending it and decides not to send
it if validation fails?
Functionality overload creeps in in small bits, with each bit being innocuous enough to not warrant a change in the name of the variable (which may involve changes across mulitple files). This results in gradual accmulation of technical debt.
At QGraph, we have a concept of "campaign", which can of several types: "regular
campaign", "trigger campaign" and "inapp campaign". (You do not need to know the details
what the campaigns are). They were implemented
in the classes RegularCampaign, TriggerCamapign and
InappCamapaign. Each of these three classes inherits from a base
class called Campaign. Of these, the regular campaign
needed a "progress updater", while the others did not.
For the sake of uniformity, we decided that all campaign would use a progress
updater, which will be passed to as an argument in the constructor of campaign
classes and through there to the constructor of class Campaign.
However, regular campaign would use an instance of class ProgressUpdater,
while trigger and inapp campaigns would use an instance of class NullProgressUpdater.
NullProgressUpdater would have the same functions exposed as the
regular ProgressUpdater, but would do nothing. That way, the functionality
of updating progress could reside in the base class Campaign and all
individual campaign classes could continue to inherit from Campaign.
This design of having two updaters ProgressUpdater and NullProgressUpdater
served us very well. We were also able use NullProgressUpdater for writing
test cases for various campaigns.
As time passed, more functionalities were added to ProgressUpdater.
It so turned out that ProgressUpdater was the only class in whole
code flow that was updating the database, and thus developers started to use
ProgressUpdater for updating the database with various statistics
and metadata. All this seemed logical too: we were happy that there was a single
gateway to the databse, and all the updates anyway seemed related, if only
tangentially, to "progress".
It so happened that once there was a progress updater related requirement which was
required by all campaigns, not just regular campaigns.
And here the developer did what he should not have done:
he added that functionality to both ProgressUpdater and
NullProgressUpdater. That was the easiest thing to do, though
not the correct thing.
Why is that bad? This is bad because now the name "NullProgressUpdater" is no longer consistent with the fact that this progress updater is actually doing some work.
We could have introduced another progress updater, say MinimalProgressUpdater,
which would have been used by campaigns other than RegularCampaign i.e.
TriggerCampaign and InappCampaign. NullCampaign
After a few more months, there was another requirement which fit well in putting the
code in ProgressUpdater. However, the requirement was so unrelated to
"progress update" that we finally decided to change the name "progress updater" to
more general "stats updater". Thus ProgressUpdater was to be renamed to
StatsUpdater and NullProgressUpdater was to be renamed
to NullStatsUpdater. However, the developer, rather than renaming the
file null_progress_udpater.py to null_stats_updater.py
decided to copy stats_updater.py to null_stats_updater.py
and then modify all the functions to no-op. He assumed that since it is null
stats updater, all the function calls should be dummy.
Thus, NullProgressUpdater went back to being true to its name. This resulted
in a bug in our code which was uncovered only around after a month when a client complained of
obviously incorrect results. Only then we then introduced the new stats updater, called
TriggerStatsUpdater.
It requires discipline and good judgement to decide when to make the extra effort of refactoring while implementing an added functionality. We lacked the good judgement in above case.
Unfortunately, just like car accidents, there is no way to avoid the bad judgement calls. The best we can do is to adopt best practices and be disciplined. And just as we expect car accidents to go down with self driving cards, coding utopia will be achieved when computers start writing code.