Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am very new to JQuery and am not familiar with all its methods. How would you refactor showNextTip() method (it looks very ugly to me)

    <div id="dialog-tip" title="Did You Know?">
        <p>I am tip 1</p>
        <img src="Images/Tips/tip1.png" alt="Tip!" />     
    </div>

function showNextTip() {
    var tip1_title = "I am tip 1";
    var tip1_p = "<p>" + tip1_title + "</p>";
    var tip1_img = "<img src='Images/Tips/tip1.png' alt='Tip!' />";

    var tip2_title = "I am tip ";
    var tip2_p = "<p>" + tip2_title + "</p>";
    var tip2_img = "<img src='Images/Tips/tip2.png' alt='Tip!' />";

    var tip3_title = "I am tip 3";
    var tip3_p = "<p>" + tip3_title + "</p>";
    var tip3_img = "<img src='Images/Tips/tip3.png' alt='Tip!' />";

    if ($("#dialog-tip p").html() == tip1_title) {
        $("#dialog-tip p").replaceWith(tip2_p);
        $("#dialog-tip img").replaceWith(tip2_img);
    } else if ($("#dialog-tip p").html() == tip2_title) {
        $("#dialog-tip p").replaceWith(tip3_p);
        $("#dialog-tip img").replaceWith(tip3_img);
    } else if ($("#dialog-tip p").html() == tip3_title) {
        $("#dialog-tip p").replaceWith(tip1_p);
        $("#dialog-tip img").replaceWith(tip1_img);
    }
}

EDIT: I might have different strings for Title and Image. in c# i would of create a class named Tips with 2 properties: title and image. how can i do so with JQuery?

share

migration rejected from stackoverflow.com Dec 15 '13 at 2:53

This question came from our site for professional and enthusiast programmers. Votes, comments, and answers are locked due to the question being closed here, but it may be eligible for editing and reopening on the site where it originated.

closed as off-topic by Jamal Dec 15 '13 at 2:53

If this question can be reworded to fit the rules in the help center, please edit the question.

    
Well, here are a couple tips: DOM manipulation is expensive. Put all the content in the DOM, then just toggle which "tip" is active using CSS classes. Cache references to DOM elements (anything like $("#dialog-tip"). Really, just do as little "DOM touching" as possible. –  Mark Oct 1 '12 at 17:07
    
This question appears to be off-topic because it only asks for new refactored code instead of ideas for refactoring the original code. –  Jamal Dec 15 '13 at 2:53

1 Answer 1

<div id="dialog-tip" title="Did You Know?">
    <p class="tip-text">I am tip 1</p>
    <img class="tip-image" src="/images/tips/tip1.png" alt="">
</div>

(function() {
    var tips = ['I am tip 1', 'I am tip 2', 'I am tip 3'],
        tipIndex = 0;

    window.showNextTip = function() {
        tipIndex = (tipIndex + 1) % tips.length;
        $('#dialog-tip .tip-text').text(tips[tipIndex]);
        $('#dialog-tip .tip-image').prop('src', '/images/tips/tip' + (tipIndex+1) + '.png');
    };
})();
  • Avoid duplication.
  • Avoid pollution of the global name space.
  • Reference elements by classes rather than tag names to improve maintainability.
  • Always escape HTML to avoid XSS and hidden bugs.
  • Minimize DOM touches, because they can be expensive.
share