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
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.

  • 2
    This is the natural approach. Grouping functions together which belong together, especially when they share common parameters is a typical "use case" for a class. – Doc Brown Apr 5 '16 at 10:46
  • Well it seems better, but the rest of the code belongs to the current file (variables referencing to markers, routes, etc.), so I have to pass them to my new Alerter class as well. – Ferenc Dajka Apr 5 '16 at 10:57
  • 2
    @FerencDajka: to me, your comment sounds even more for a reason to clean up the structure of your code by introducing a class with a well-defined input and output. – Doc Brown Apr 5 '16 at 14:02
  • I'm fuzzy on how Alerter has access to the original code's changeMarkerOpacityIfNeeded(),alertMarker etc. – user949300 Nov 3 '16 at 18:18
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]);
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.