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.

My code blinks text for an interval of time then clears the text.

I plan to use this code in a quiz web app. When the user gets an answer right, "CORRECT!" will blink on the screen for 10 seconds. When the user gets an answer wrong, "INCORRECT!" will blink on the screen for 10 seconds.

I'm looking for feedback on how I could simplify and improve it?

JS Bin: http://jsbin.com/ojetow/1/edit

<div id="blinkText"></div>

<script>
// Takes text to blink and id of element to blink text in
function blinkText(text, id) {
    // Blink interval
    setInterval(blinker, 250);

    // Flag to see what state text is in (true or false)
    var flag = true;

    // Number of times to blink text
    var blinkNum = 10;
    var i = 1;
    var divID = document.getElementById(id);

    function blinker() {
        if (i < blinkNum) {
            if (flag) {
                divID.innerHTML = text;
                flag = false;
            } else {
                divID.innerHTML = "";
                flag = true;
            }

            i++;
        } else {
            // Delete if it's still showing
            divID.innerHTML = "";

            // Stop blinking
            clearInterval(blinker);
        }
    }
}

blinkText("Hello World", "blinkText");
</script>
share|improve this question
    
I would suggest using a library, for example: docs.jquery.com/UI/Effects/Pulsate –  abuzittin gillifirca Jan 31 '13 at 15:08
add comment

1 Answer 1

up vote 3 down vote accepted

It doesn't look very complex to me but the variable names could use some attention, as they're not very descriptive. I'd suggest:

  • textHidden instead of flag
  • timesBlinked instead of i
  • targetElement instead of divID (it's not an id anymore, but the element referred to by the id)

I would also toggle the element visibility instead of removing and adding the text for several reasons, one of them being that removing text can also change the element's height and/or width, which can cause problems with the layout.

if( textHidden ) {
    targetElement.style.visibility = 'visible';
} else {
    targetElement.style.visibility = 'hidden';
}

textHidden = !textHidden;  // flip the flag
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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