Code Bloat Commandos looks at why software code deteriorates over time; and how to fight back.
In this episode, we will look at how a nice and clean method grows into a dumping ground function. We will see when it happens, how to spot it and how to fix it.
FUNCTIONS SHOULD DO ONE THING. THEY SHOULD DO IT WELL. THEY SHOULD DO IT ONLY.
The humble beginnings
Project specification:
- We have a live video player and an event function called
onProgress
that is executed every second. - We can use that function to monitor playback progress and hook up various actions.
- Our job is to implement a function that will track video engagement by sending an analytics ping every 30 seconds.
This is our code (JavaScript):
// Main entry point. This function will be called every second by the player |
This is our first version of the _sendVideoEngagementAnalytics
function. It is still reasonably simple and compact. It checks if more than 30 seconds have elapsed. If it did, it sends an analytics ping using an analytics manager object.
A few weeks later
Our first version of the app is working and a few weeks later we need to also check if the current content playing is an Advertisement; and if that is the case send an Advertisement Start and End pings.
We can check if the current content playing is an Ad by calling isAdPlaying(currentTime)
.
Easy enough, let’s get coding.
// Main entry point. This function will be called every second by the player |
Done. git commit -m "Added Ad tracking analytics to player"
. Jump on Skype and ask who is ready for lunch.
This is (almost) what happened to me today.
The bloat
The code is not that bad, we have:
- refactored
analyticsData
out of the initial if block because of variable hoisting and scoping in JavaScript. - created a local variable
adPlaying
to avoid multiple calls toisAdPlaying(currentTime)
.
### These are great, but we have committed an even bigger mistake here:
The _sendVideoEngagementAnalytics
is now doing 2 things; which is pretty bad in and of itself but also has the following scary consequences:
- The name
_sendVideoEngagementAnalytics
is now incorrect (we are not only monitoring video engagement but ad playback as well). Someone reading_sendVideoEngagementAnalytics(currentTime);
in the calling function might not suspect that it also sends Ad related analytics. - The function is getting longer. And we are setting the stage for it to become even longer by starting off a series of if blocks that check playback position related stuff.
- We suddenly feel like we need comments in our code to explain what it does and ‘structure’ the two parts of our function. Burp.
Pain Relief
Recognising the problem
It clicked for me when I realised the function’s name didn’t correspond to what the function actually does.
Another hint could have been the fact that the function now has two sections of code that handles different business requirements.
A function that is 30 lines long and needs comments for ‘if conditions’ that are pretty readable is not a good sign either.
But really, simply observing that the function I have modified suddenly does not respect the Single Responsibility Principle should have been enough.
Administering the cure
At this point, the solution is pretty clear. Separating the ravioli.
Let’s go ahead and split our function into two functions:
function onProgress(currentTime) { |
Following up after treatment
When reading
_sendVideoEngagementAnalytics(currentTime);
_sendAdContentAnalytics(currentTime);we know exactly what is going on. No extra ‘hidden’ functionality.
No more ugly comments
Shorter functions
If someone wants to add more analytics logic, they won’t be tempted to add to that function and worsen the problem. They will do the right thing right away by adding a new function.