3
\$\begingroup\$

I have a webpage that shows shorthand code on the left, and its long form on the right. When you hover over code on the left (signified with a space-separated array in the data-from attribute), it uses jQuery to highlight corresponding code on the right (signified with a space-separated array in the data-to attribute), and vice-versa. It's working great, but it feels very long.

Script

function containsAny(haystack, needles) {
    for(needle in needles)
        if (haystack.indexOf(needles[needle]) >= 0)
            return true;
}
$(document).ready(function() {
    /* from to */
    $("[data-from]").each(function(i, e) {
        var $e = $(e),
        froms = $e.data("from").toString().split(/\s+/); // must do toString in case we get an int
        $e.hover(function() {
            $e.addClass("hilighted");
            $("[data-to]").each(function(i2, e2) {
                var $e2 = $(e2),
                tos = $e2.data("to").toString().split(/\s+/);
                if (containsAny(tos, froms))
                    $e2.addClass("hilighted");
            });
        },
        function() {
            $("[data-to]").each(function(i2, e2) {
                $e.removeClass("hilighted");
                var $e2 = $(e2),
                tos = $e2.data("to").toString().split(/\s+/);
                if (containsAny(tos, froms))
                    $e2.removeClass("hilighted");
            });
        });
    });

    /* to from */
    $("[data-to]").each(function(i, e) {
        var $e = $(e),
        tos = $e.data("to").toString().split(/\s+/);
        $e.hover(function() {
            $e.addClass("hilighted");
            $("[data-from]").each(function(i2, e2) {
                var $e2 = $(e2),
                froms = $e2.data("from").toString().split(/\s+/);
                if (containsAny(froms, tos))
                    $e2.addClass("hilighted");
            });
        },
        function() {
            $("[data-from]").each(function(i2, e2) {
                $e.removeClass("hilighted");
                var $e2 = $(e2),
                froms = $e2.data("from").toString().split(/\s+/);
                if (containsAny(froms, tos))
                    $e2.removeClass("hilighted");
            });
        });
    });
});

SSCCE HTML

<H3>Before</H3>
<PRE>
<SPAN DATA-FROM="link-color">*:link</SPAN> {
    <SPAN DATA-FROM="link-color">color: blue;</SPAN>
}

<SPAN DATA-FROM="btn-color">button,
input[type=("button","submit","reset")]</SPAN> {
    <SPAN DATA-FROM="btn-color">color: match(*:link);</SPAN>
}
</PRE>

<H3>After</H3>
<PRE>
<SPAN DATA-TO="link-color">*:link,</SPAN>
<SPAN DATA-TO="btn-color">button,
input[type="button"],
input[type="submit"],
input[type="reset"]</SPAN> {
    <SPAN DATA-TO="link-color btn-color">color: blue;</SPAN>
}
</PRE>

Full use case

How can I make the JavaScript shorter (or at least more OO) without sacrificing functionality? I'm sure it can be shorter or more OO because it's so redundant, but I can't figure how.

\$\endgroup\$
1
\$\begingroup\$

I'm not sure its either shorter or easier to read but here is one version:

JSFIDDLE

$(document).ready(function() {
    var containsAny = function(haystack, needles) {
        for(needle in needles)
            if (haystack.indexOf(needles[needle]) >= 0)
                return true;
    }

    var whiteSpaceRegexp = /\s+/;
    var isFromString = function( isFrom ){
        return isFrom ? "from" : "to";
    };
    var splitData = function( $element, isFrom ){
        return $element.data( isFromString( isFrom )  ).toString().split( whiteSpaceRegexp );
    };

    var createToggleFunction = function( $element, classMethodName, searches, isFrom ){
        return function(  ){
            $element[classMethodName]( "hilighted" );
            $("[data-" + isFromString( isFrom ) + "]").each( 
                function( index, innerElement ) {
                    var $innerElement = $(innerElement);
                    var words = splitData( $innerElement, isFrom );
                    if (containsAny(words, searches ))
                        $innerElement[classMethodName]("hilighted");
                }
            );
        };
    };

    var createHoverFunction = function( isFrom )
    {
        return function( index, element ) {
            var $element = $(element);
            var searches = splitData( $element, isFrom );
            $element.hover(
                createToggleFunction( $element, "addClass", searches, !isFrom ),
                createToggleFunction( $element, "removeClass", searches, !isFrom )
            );
        }
    }

    $("[data-from]").each( createHoverFunction( true ) );
    $("[data-to]").each( createHoverFunction( false ) );
});

Or, if you want code golf, then this is much shorter (and definitely very hard to read):

JSFIDDLE

$(document).ready(function(){var c=function(h,n){for(x in n)if(h.indexOf(n[x])>=0)return true},w=/\s+/,s=function(f){return f?"from":"to"},p=function(e,f){return e.data(s(f)).toString().split(w)},t=function(e,n,x,f){return function(){e[n]("hilighted" );$("[data-"+s(f)+"]").each(function(_,i){var I=$(i);if (c(p(I,f),x))I[n]("hilighted")})}},H=function(f){return function(_,e) {var E=$(e),s=p(E,f);E.hover(t(E,"addClass",s,!f),t(E,"removeClass",s,!f));}};$("[data-from]").each(H(1));$("[data-to]").each(H(0))});
\$\endgroup\$
  • 1
    \$\begingroup\$ This is not much of a review. Consider commenting on specific issues or parts you'd change, and why. \$\endgroup\$ – raptortech97 Nov 18 '14 at 1:56
  • \$\begingroup\$ wow this is beautiful! Much more like what I envisioned going in! \$\endgroup\$ – Supuhstar Nov 19 '14 at 20:50
2
\$\begingroup\$

Here's your code, and I've pointed out some potential problems:

for(needle in needles)
    if (haystack.indexOf(needles[needle]) >= 0)
        return true;

While this is possible, I suggest you use {} instead. This avoids ambiguity, and future danger, especially when you'll be adding more functionality beyond just a single line.

Additionally, for-in runs over prototype properties. If you happen to have a property that is similarly named as a native property in the prototype, you'll be in trouble. To avoid that, check using hasOwnProperty before proceeding with the code.

$(document).ready(function() {

Probably not better, but there's a shorthand for this: $(function(){...})

$("[data-from]").each(function(i, e) {

I'd probably avoid using attribute selectors. They're known to be slow, depending on the parser implementation. I suggest you stick to classes.

froms = $e.data("from").toString().split(/\s+/); // must do toString in case 

A shorthand to convert stuff to string is to do '' + to it. Not so verbose, but it's somewhat elegant and short.

\$\endgroup\$
  • \$\begingroup\$ can you elaborate more on your paragraph about the use of hasOwnProperty in for-in? Perhaps provide an example? \$\endgroup\$ – Supuhstar Nov 19 '14 at 20:35

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.