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.
- We have a live video player and an event function called
onProgressthat 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 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.
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
Easy enough, let’s get coding.
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.
- created a local variable
adPlayingto avoid multiple calls to
_sendVideoEngagementAnalytics is now doing 2 things; which is pretty bad in and of itself but also has the following scary consequences:
- The name
_sendVideoEngagementAnalyticsis 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.
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.
At this point, the solution is pretty clear. Separating the ravioli.
Let’s go ahead and split our function into two functions:
- When reading12_sendVideoEngagementAnalytics(currentTime);_sendAdContentAnalytics(currentTime);
we know exactly what is going on. No extra ‘hidden’ functionality.
No more ugly comments
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.