Have you ever found yourself stuck in a situation where you have more questions than lines of code? Probably every developer can relate to the title of this blog. So, take a cup of coffee and “enjoy” the process I had to go through while working on this problem.

Recently I was doing some bug fixing and after a while I found this piece of code as a potential source of the problem:

volumeOnTouch() {

    const speakerFlag = Session.get(“speakerFlag”);

    if (typeof speakerFlag !== “undefined” && speakerFlag.state) {

        speakerFlag.touched = true;

        speakerFlag.state = false;

        Session.set(“speakerFlag”, speakerFlag);

        Session.setPersistent(“speakerRestModeFlag”, false);

        systemUtility.volumeLevel(“30”);

        setTimeout(function() {

            StatusLedHelper.whiteLEDOn();

            const stationsService = new StationsService();

            stationsService.findCurrentStation();

            stationsService.findNextStation();

        }, 100);

    }

}

As you can see, it immediately poses more questions than lines of code. So let’s go through it. 

• What should this method do? 
• Is this method used to set volume level?
• If it is, why doesn’t it receive a volume level value? 
• What is “the flag” of the speaker?
• Also what is the state of the speaker’s flag?
• Can we touch the speaker’s flag somehow?
• Why is “speakerFlag” set before the function finishes its work?
• What does it mean when there is a “false” set as a value of “speakerRestModeFlag”? Is it the absence of that flag?
• Why does this function set the volume level and then set “timeout” to change the color of the LED?
• How is it secured that the asynchronous call with “setTimeout” will not overwrite the color of the LED that may have been changed in this 100 millisecond?

And lastly, one of my biggest concerns;

• Why are the results of “findCurrentStation()” and “findNextStation()” functions not used?

From the function names I was expecting that these two functions would return values. 

Let’s dig deeper into the problem

Both functions return a Boolean result, which is quite unexpected for functions looking for “current” and “next” station. Somewhere in the code they set the Meteor Session values with Current and Next station. These are the side effects of these functions and something totally unexpected, this could lead to various unpredictable behaviors in the application. Therefore it’s recommended to avoid any side effect.

The biggest issue of this piece of code and related functions is the expensive maintenance. Developers who have to debug it, add or change some functionalities need to go through each and every line of code and get the overall picture of how it needs to work. After this long evaluation process it is a difficult and time-consuming process of narrowing down the source of the bug or examining which existing function that could be reused.

As a developer we always have to strive to make code of good quality that is easy to maintain. In order to achieve this we should follow practices that are proven to be good and used by many developers with great experience. 

Let’s try to break out some of the problems noted in this piece of code . The function “volumeOnTouch” needs  a better name. In order to pick a good name for the function we need to try to describe what it does. This function serves to turn on the speaker, change the color of the LED, and to populate the session with stations after the speaker is awakened from stand-by mode. Clearly, there are three things going on here. So maybe we can name this function wakeUpSpeaker and call the three other functions, that have single responsibilities, something like this:

wakeUpSpeakerIfSleeping() {

    if (this.isSpeakerSleeping()) {

        this.clearPutSpeakerToSleepTimeout();

        this.setDefaultVolumeLevel();

        this.changeSpeakerStatusToWakeUp();

        this.wakeUpSpeakerTimeout = setTimeout(() => {

            this.setStatusLedColorOnWakeUp();

            this.setStationsInSession();

        }, 100);

    }

}

isSpeakerSleeping() {

    const speakerStatus = Session.get(“speakerStatus”);

    return (typeof speakerStatus !== “undefined” && speakerStatus.sleeping === true);

}

 

clearPutSpeakerToSleepTimeout() {

    if (this.putSpeakerToSleepTimeout) {

        clearTimeout(this.putSpeakerToSleepTimeout);

        this.putSpeakerToSleepTimeout = null;

    }

}

setDefaultVolumeLevel() {

    SystemUtility.setVolumeLevel(Constants.DEFAULT_VOLUME_LEVEL);

}

 

changeSpeakerStatusToWakeUp() {

    Session.set(“speakerStatus”, { touched: true, sleeping: false });

    Session.set(“speakerRestModeOn”, false);

}

setStatusLedColorOnWakeUp() {

    StatusHelper.setColor(StatusHelper.STATUS_LED, StatusHelper.COLORS.WHITE);

}

setStationsInSession() {

    const stationsService = new StationsService();

    stationsService.setCurrentStationInSession();

    stationsService.setNextStationInSession();

}

Now it’s easier to follow the logic and you can have a clear expectation on what the function is supposed to do. Also, there is the same level of abstraction used in each function. Session variables and other functions have appropriate names which are now more meaningful. Still in this code the Meteor Session is manipulated from various places. In my opinion this is not a good practice. “StationService“ class should not be aware of Meteor Session.

“Functions should do one thing. They should do it well. They should do it only.” (From the book of R. Martin) If you achieve this then you have a code that is easy to test and easy to maintain. “You know you are working on clean code when each routine turns out to be pretty much what you expected.” (Ward Cunningham)

Code that is not fully covered with tests and has a complex logic to follow will put an extra pressure on developers when they are, for example, fixing bugs. They cannot be sure that fixing one problem will not produce another one somewhere else in the code. Purpose of having “timeout” in this example is not clear, moreover that is the cause of the bug. Because I wasn’t comfortable removing this timeout, I have introduced variable wakeUpSpeakerTimeout to control the execution of it and to fix the bug (variable is used in another method which will put the speaker to sleep). 

The Meteor Session is really powerful and you can use and introduce it anywhere from the front end part of the application. I can see that the Meteor Session is too often abused. As Pirelli said: “Power is nothing without control”. In my opinion the Meteor Session should be strictly controlled through a few exposed functions. 

When you are using Redux or React Context for state management then you have to be explicit on how you provide and use that state variables. So, I would recommend to follow the same principle when using the Meteor Session because in that case you take more care when introducing new variables and how you use them.

Following these recommendation we can refactor application to set the Meteor Session variables like this:

setStationsInSession() {

    const stationService = new StationService();

    MeteorStateHelper.setVariableValue(

        MeteorStateHelper.VARIABLES.CURRENT_STATION,

        stationService.getCurrentStation()

    );

    MeteorStateHelper.setVariableValue(

        MeteorStateHelper.VARIABLES.NEXT_STATION,

        stationService.getNextStation()

    );

}

“ setVariableValue “ function should do a data validation in order to get the most benefit out of it.  

Coding takes time. When you get your code to work, take a break and then look at it with fresh eyes. We are constantly in a hurry and sometimes tired and usually we overlook code that is not easy to maintain and that is not understandable to other developers. Find your pace and always follow best practices. There are a lot of good books and resources available online regarding best practices. I would like to recommend the book: Clean Code: A Handbook of Agile Software Craftsmanship by Robert C. Martin.

Happy coding.

/Eldar Kapetanovic