3
\$\begingroup\$

Hey so I have written some code out in jQuery, but it seems to violate DRY. I'm not too familiar with JavaScript or jQuery, so I was just asking for some advice on how to optimize this code:

$("#twitter").rotate({
  bind:
  {
    mouseover : function() {
    $(this).rotate({animateTo:360});

    $("#twitter").fadeTo(function() { 
        $(this).load(function() { $(this).fadeTo(); });
        $(this).attr("src", "images/logos/Twitter-Color.png");
    });
  },
  mouseout : function() {
    $(this).rotate({animateTo:0})
    $("#twitter").fadeTo(function() { 
        $(this).load(function() { $(this).fadeTo(); }); 
        $(this).attr("src", "images/logos/Twitter-White.png");
    });
  }
  }
});

$("#linkedin").rotate({
  bind:
  {
    mouseover : function() {
    $(this).rotate({animateTo:360});

    $("#linkedin").fadeTo(function() { 
        $(this).load(function() { $(this).fadeTo(); });
        $(this).attr("src", "images/logos/Linkedin-Color.png");
    });
  },
  mouseout : function() {
    $(this).rotate({animateTo:0})
    $("#linkedin").fadeTo(function() { 
        $(this).load(function() { $(this).fadeTo(); }); 
        $(this).attr("src", "images/logos/Linkedin-White.png");
    });
  }
  }
});
$("#facebook").rotate({
  bind:
  {
    mouseover : function() {
    $(this).rotate({animateTo:360});

    $("#facebook").fadeTo(function() { 
        $(this).load(function() { $(this).fadeTo(); });
        $(this).attr("src", "images/logos/Facebook-Color.png");
    });
  },
  mouseout : function() {
    $(this).rotate({animateTo:0})
    $("#facebook").fadeTo(function() { 
        $(this).load(function() { $(this).fadeTo(); }); 
        $(this).attr("src", "images/logos/Facebook-White.png");
    });
  }
  }
});
\$\endgroup\$
  • \$\begingroup\$ Is the code you posted here indented as intended? \$\endgroup\$ – 200_success Jan 13 '16 at 16:22
1
\$\begingroup\$

Styling and readability

Your indentation is incorrect. This makes it harder to read, and makes people assume that "mouseout" belongs to a different Object that it actually does.

Whitespace is not always consistent (e.g. $(this).rotate({animateTo:360}); versus other uses of Objects.

DRY

As you correctly noted, this code repeats itself quite a bit.

The code seems to only differ in id and logo name. You have various options:

  • You create a function that sets this. I am not entirely sure what you would name it, but something like setRotationEffects( id, logoName ); might do the trick.
  • You can use named functions for mouseout and mouseover. This will leave you with a basic boilerplate for each id:

    $("#linkedin").rotate({
      bind:
      {
        mouseover : whatEverYouCallIt,
        mouseout  : whatEverOtherThingYouCallIt
      }
    });
    

    This might be more optimal if you expect to have other similar constructs with different effects.

Similarly:

  • The id in the mouseover and mouseout handler is not needed. In the handler you can use $(this) instead. This leaves you with only the name of the logo.
    • You can consider pulling this name from the existing logo. After all, the logo was set to a particular name in the first place. This makes all these identical.
    • You can use an anonymous function containing a function call with parameters (function() { doTheThings( id, logoName ); }.

Note: It might be just me, but I link "optimize" to "performance", and DRY has nothing to do with performance. We use DRY to make maintenance easier, not to make code outperform "WET" code.

\$\endgroup\$
  • \$\begingroup\$ Nice answer. "Optimization" is broader than just performance, though ;-) \$\endgroup\$ – Mathieu Guindon Nov 10 '16 at 14:31

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.