2
\$\begingroup\$

I run into this often. I have two functions very similar, that if I combine would be more DRY but if I didn't would be easier to read and make more sense.

In this case I have two implementations of fade, one using setInterval and one using setTimeout.

How do I not get stuck deciding between Dry vs. readability. I hope this is not an opinion question that is not answerable.

   /*fade
    **  d:none
    **  n:uses setTimeout
    **
    */
    NS.prototype.fade = function (direction, max_time) {
        var statics = {},
            elements = this;
        statics.elapsed = 0;
        statics.granularity = 10;
        if (statics.timeout_id) {
            clearTimeout(statics.timeout_id);
        }
        (function next() {
            var kindex,
                opacity;
            statics.elapsed += statics.granularity;
            if (direction === 'up') {
                for (kindex in elements) {
                    if (elements.hasOwnProperty(kindex)) {
                        opacity = statics.elapsed / max_time;
                        elements[kindex].style.opacity = opacity;
                    }
                }
            } else if (direction === 'down') {
                for (kindex in elements) {
                    if (elements.hasOwnProperty(kindex)) {
                        elements[kindex].style.opacity = (max_time - statics.elapsed) / max_time;
                    }
                }
            }
            if (statics.elapsed < max_time) {
                statics.timeout_id = setTimeout(next, statics.granularity);
            }
        }());
    };

    /*fadeL
    **  d:none
    **  n:(L)inear. Uses setInterval. Better for short animations.
    **
    */
    NS.prototype.fadeL = function (direction, max_time) {
        var statics = {},
            elements = this;
        statics.elapsed = 0;
        statics.granularity = 10;
        if (statics.timeout_id) {
            clearInterval(statics.timeout_id);
        }
        (function next() {
            var kindex,
                opacity;
            statics.elapsed += statics.granularity;
            if (!statics.timeout_id) {
                statics.timeout_id = setInterval(next, statics.granularity);
            }
            if (direction === 'up') {
                for (kindex in elements) {
                    if (elements.hasOwnProperty(kindex)) {
                        opacity = statics.elapsed / max_time;
                        elements[kindex].style.opacity = opacity;
                    }
                }
            } else if (direction === 'down') {
                for (kindex in elements) {
                    if (elements.hasOwnProperty(kindex)) {
                        elements[kindex].style.opacity = (max_time - statics.elapsed) / max_time;
                    }
                }
            }
            if (statics.elapsed > max_time) {
                clearInterval(statics.timeout_id);
            }
        }());
    };
\$\endgroup\$
0

1 Answer 1

8
\$\begingroup\$

It's generally a good idea to remove duplication wherever possible. If you have two functions that are logically similar, but don't share code, you run the risk of code rot - updating one function and forgetting to apply similar changes to the other.

There are a few ways I would suggesting working around this without sacrificing too much readability.

  1. Find common parts of each function and refactor them out into their own sub-functions
  2. Use higher-order functions to inject the unique logic into a more generic base function

I hardly know Javascript at all, so I can't be too specific on how to handle this case, but it looks like you only have a very small amount of code that isn't shared.

if (statics.timeout_id) {
    clearTimeout(statics.timeout_id); // fade
    clearInterval(statics.timeout_id); // fadeL
}

// .. snip ..

// Added in fadeL
if (!statics.timeout_id) {
    statics.timeout_id = setInterval(next, statics.granularity);
}

// .. snip ..

if (statics.elapsed < max_time) { // fade
if (statics.elapsed > max_time) { // fadeL
    statics.timeout_id = setTimeout(next, statics.granularity); // fade
    clearInterval(statics.timeout_id); // fadeL
}

A really simple approach would be to make a base function that accepts a boolean value to differentiate between the slight changes in behavior:

NS.prototype.base_fade = function (direction, max_time, is_fade_l) {
    var statics = {},
        elements = this;
    statics.elapsed = 0;
    statics.granularity = 10;
    if (statics.timeout_id) {
        if (is_fade_l) { 
            clearInterval(statics.timeout_id); 
        } else { 
            clearTimeout(statics.timeout_id); 
        }
    } 
    (function next() {
        var kindex,
            opacity;
        statics.elapsed += statics.granularity;
        if (is_fade_l && !statics.timeout_id) {
            statics.timeout_id = setInterval(next, statics.granularity);
        }
        if (direction === 'up') {
            for (kindex in elements) {
                if (elements.hasOwnProperty(kindex)) {
                    opacity = statics.elapsed / max_time;
                    elements[kindex].style.opacity = opacity;
                }
            }
        } else if (direction === 'down') {
            for (kindex in elements) {
                if (elements.hasOwnProperty(kindex)) {
                    elements[kindex].style.opacity = (max_time - statics.elapsed) / max_time;
                }
            }
        }
        if (is_fade_l) {
            if (statics.elapsed > max_time) {
                clearInterval(statics.timeout_id);
            }
        } else {
            if (statics.elapsed < max_time) {
                statics.timeout_id = setTimeout(next, statics.granularity);
            }
        }
    }());
};

Then call that function from fade and fadeL providing the correct boolean. This way, at least, you have a guarantee that the code that is logically the same will stay the same. It also takes up less space in your codebase, and doesn't require all that much more parsing to understand it mentally.

You might be able to get more modular with this, and refactor some elements out into their own functions, but my own lack of knowledge of the language and the codebase means I'm not quite up to suggesting those changes. In general though, try to consolidate logically equivalent code wherever you can. It will make your life easier in the long run.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.