#Software Practices #Single Responsibility Principle

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
function onProgress(currentTime) {
    
    // [...] Do UI stuff here like update progress bar and set Time
    
    // Our function
    _sendVideoEngagementAnalytics(currentTime);
    
    // [...] Do more things here like update the icons in the footer
}

function _sendVideoEngagementAnalytics(currentTime) {
    
    if(Math.abs(currentTime - __analyticsState.fromPosition) > 30) {
    
        var analyticsData = {
            event: "VIDEO_PROGRESS"
        };
        
        analyticsManager.send(analyticsData);   
        
        __analyticsState.fromPosition = currentTime;
    }
}

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
function onProgress(currentTime) {
    
    // [...] Do UI stuff here like update progress bar and set Time
    
    _sendVideoEngagementAnalytics(currentTime);
    
    // Do more things here like update the footer controls
}

function _sendVideoEngagementAnalytics(currentTime) {
    
    var analyticsData,
        adPlaying = isAdPlaying(currentTime);
    
    // Checks if more that 30 seconds have passed
    if(Math.abs(currentTime - __analyticsState.fromPosition) > 30) {
    
        analyticsData = {
            event: "VIDEO_PROGRESS"
        };
        
        analyticsManager.send(analyticsData);   
        
        __analyticsState.fromPosition = currentTime;
    }
    
    // Checks if an Ad just started playing
    if(__analyticsState.adPlaying !== adPlaying) {
        
        analyticsData = {
            event: adPlaying ? "AD_BEGIN" : "AD_COMPLETE"
        };
        
        analyticsManager.send(analyticsData);   
        
        __analyticsState.adPlaying = adPlaying;
    }
}

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:


### 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) {
    
    // [...] Do UI stuff here like update progress bar and set Time
    
    _sendVideoEngagementAnalytics(currentTime);
    _sendAdContentAnalytics(currentTime);
    
    // Do more things here like update the footer controls
}

function _sendVideoEngagementAnalytics(currentTime) {
    
    var analyticsData;
    
    if(Math.abs(currentTime - __analyticsState.fromPosition) > 30) {
    
        analyticsData = {
            event: "VIDEO_PROGRESS"
        };
        
        analyticsManager.send(analyticsData);   
        
        __analyticsState.fromPosition = currentTime;
    }
}

function _sendAdContentAnalytics(currentTime) {
    
    var analyticsData,
        adPlaying = isAdPlaying(currentTime);
    
    if(__analyticsState.adPlaying !== adPlaying) {
        
        analyticsData = {
            event: adPlaying ? "AD_BEGIN" : "AD_COMPLETE"
        };
        
        analyticsManager.send(analyticsData);   
        
        __analyticsState.adPlaying = adPlaying;
    }
}

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.


#Software Practices #Single Responsibility Principle