4

So Clean Code says you should separate each task to a single function (and add these functions a correct name).

I like the idea, but I've faced this problem so far: you receive a parameter which you want to use also on the topmost function, but you need it on the lowest level as well, so you have to pass the parameter to every intermediate function, which is not cool.

Here's my code:

onUpdate = function(currentSeconds){
    updateRoute(currentSeconds);
    updateAlerts(currentSeconds);
};
updateAlerts = function(currentSeconds){
    for(var i = 0; i < alertMarkers.length; ++i)
        oneAlertUpdate(currentSeconds, alertMarkers[i], alertAll[i]);
}
oneAlertUpdate = function(currentSeconds, alertMarker, alert){
        updateAlertMarker(alertMarker, alert[entryIndex]);
        changeMarkerOpacityIfNeeded(alertMarker, currentSeconds);
    }
}
changeMarkerOpacityIfNeeded = function(alertMarker, currentSeconds){
    //set opacity based on currentSeconds
    var opacity = alertMarker.opacityTime < currentSeconds ? 1 : 0.5;
    if(alertMarker.getOpacity() != opacity)
        alertMarker.setOpacity(opacity);
}
  • the parameter here is currentSeconds it has to be passed, but it's only used on the lowest level
  • please note that I don't use currentSeconds directly in the topmost function, but it's crucial to be passed there, I'll write the reason if you say it's relevant
Ferenc Dajka
  • 175
  • 3

2 Answers2

2

One option could be to create a "class" (or the JavaScript equivalent in this case), which behaves like a functionoid/functor, and stores currentSeconds, then call into that 'class'[sic] from a proxy function to make sure that you're still only calling onUpdate from the calling code:

var Alerter = (function () {
    function Alerter(currentSeconds) {
        this.currentSeconds = currentSeconds;
    }

    Alerter.prototype.onUpdate = function () {
        this.updateRoute();
        this.updateAlerts();
    };

    Alerter.prototype.updateAlerts = function () {
        for (var i = 0; i < alertMarkers.length; ++i)
            this.oneAlertUpdate(alertMarkers[i], alertAll[i]);
    };

    Alerter.prototype.oneAlertUpdate = function (alertMarker, alert) {
        this.updateAlertMarker(alertMarker, alert[entryIndex]);
        this.changeMarkerOpacityIfNeeded(alertMarker);
    };

    Alerter.prototype.changeMarkerOpacityIfNeeded = function (alertMarker) {
        //set opacity based on currentSeconds
        var opacity = alertMarker.opacityTime < this.currentSeconds ? 1 : 0.5;
        if (alertMarker.getOpacity() != opacity)
            alertMarker.setOpacity(opacity);
    };

    return Alerter;
}());


function onUpdate(currentSeconds) {
    new Alerter(currentSeconds).onUpdate();
}

The main disadvantage to this is more additional complexity in the code, which just negates most of the benefit in avoiding repeatedly passing the same variable(s) around in the first place.

However it avoids that bit of duplication, and the calling code should not notice any difference at least:

var currentSeconds = 5;
onUpdate(currentSeconds);

But overall, I'm not sure whether this approach is really any "cleaner"; the simplest solutions are often the best ones.

Ben Cottrell
  • 12,133
0

Do functions like oneAlertUpdate and changeMarkerOpacityIfNeeded have to be visible outside updateAlerts? If not, it could be easy to avoid having to "pass" the values down the call chain.

onUpdate = function(currentSeconds){
    updateRoute(currentSeconds);
    updateAlerts(currentSeconds);
};

updateAlerts = function(currentSeconds){
    oneAlertUpdate = function(alertMarker, alert){
            updateAlertMarker(alertMarker, alert[entryIndex]);
            changeMarkerOpacityIfNeeded(alertMarker);
        }
    }
    changeMarkerOpacityIfNeeded = function(alertMarker){
        //set opacity based on currentSeconds
        var opacity = alertMarker.opacityTime < currentSeconds ? 1 : 0.5;
        if(alertMarker.getOpacity() != opacity)
            alertMarker.setOpacity(opacity);
    }

    for(var i = 0; i < alertMarkers.length; ++i)
        oneAlertUpdate(alertMarkers[i], alertAll[i]);
}