Reading 8 divs across the screen depending on assignment time

Question

This code takes 8 divs and races them across the screen depending on which time value the div was assigned. How can I have coded this better so my code doesn't look so amateurish?

I know I should have used something other than a hard coded 8 in my for-loop, but timeArray.length is unavailable since I am removing items from the array with splice.

$(document).ready(function() {
var timeArray = new Array(3,4,5,6,7,8,9,10);
var shortestTime = timeArray[7];
var fastestPony = {};
var index;

var pony = {
    name: "pony",
    raceTime: -1,
    selected: ""
};

//change the color of the pony when the user clicks on it
$('.pony').bind('click', function() {
    $('.pony').removeClass('selectedPony');
    $(this).addClass('selectedPony');

    //get the pony that the user selected
    pony.selected = $(this);
});

$('#startButton').click(function() { 
    if (pony.selected == "") {
        alert("Please click the pony you think will win the race.");
    }
    else {
        for (i = 1; i <= 8; i++) {
           //get a random number from the timeArray
            index = Math.floor(Math.random() * timeArray.length);
            pony.raceTime = timeArray[index];

            //pull the random race time number out of the array 
            //so it can't be assigned to another horse
            timeArray.splice(index, 1);

            //get the fastest pony
            if (pony.raceTime < shortestTime) {
                shortestTime = pony.raceTime;
                fastestPony = $('#pony' + i);
            }

            //award the winner after the ponies have reached the finish line
            if (i == 8) {
                fastestPony.addClass('winner').append(' - Winner!');
            }

            //send the horses on their way to race!
            $('#pony' + i).animate({left: '320px'}, pony.raceTime * 1000);
        }
    }
});

//reset the ponies back to the starting line by reloading the page
$('#resetButton').click(function() {
    document.location.reload(true);
});
});

Answer 1

Note: As I type "pony" and "ponies" in my code, I can't help but wonder what the hell I'm doing with my time. Never thought I'd be writing words like that in javascript...

Anyway! My first suggestion would be to make everything more flexible bynot relying on element IDs, and a set number of DIV elements. If you add or remove one, you'll have to make sure its ID is numbered correctly, and update your code to match. Much easier to just use jQuery to find and animate the DIVs, regardless of how many there are.

In fact, the elements plus jQuery can tell you everything you need to know, so there's no need to store anything along the way.

Also, the reset functionality is trivial. No need reload the entire page just for that.

Lastly, and perhaps most importantly, you code announces the winner immediately - before the poni-- ahem, the DIVs have even raced. Yes, the winner is picked right away, and it's kinda' obvious which one will reach the goal first, but still. Better to announce the winner when it crosses the finish line.

Of course, overall it'd be more fun to have something more random, where the DIVs move in fits and starts, so you can't tell who's going to win until the very end. But I'll stick to refactoring your current code.

Here's a demo of my version and here's the code:

$(function() { // just pass in a function; it's the same as using $(document).ready()
  var ponies = $(".pony"), // Again: I never thought I'd write "ponies" in code
      raceButton = $("#raceButton"); // use a single button for race and reset

  ponies.click(function (event) {
    ponies.removeClass('selected');
    $(this).addClass('selected');
    // no need to store the selection; we can always find it, now that it's got a "selected" class
  });

  // use one button for both start and reset
  raceButton.click(function() {
    // If we have a winner, the race is pretty much over
    // If we have no winner, and no elements currently animating, we're ready to race
    // Otherwise we're racing; don't do anything

    if( ponies.filter(".winner").length !== 0 ) {
      reset();
    } else if( ponies.filter(".winner, :animated").length === 0 ) {
      race();
    }
  });

  function race() {
    // check for selection
    if( ponies.filter(".selected").length === 0 ) {
      alert("Please click the pony you think will win the race.");
      return; // return here instead of having a big else-block
    }

    var times  = [], i, l;

    // disable the race button
    raceButton.prop("disabled", true);

    // build time array; don't hardcode it
    for( i = 0, l = ponies.length ; i < l ; i++ ) {
      times.push(i);
    }

    // Use .each() instead of relying on all the divs having numbered IDs.
    ponies.each(function () {
      var index = Math.floor(Math.random() * times.length),
          time  = times[index],
          pony  = $(this);

      times.splice(index, 1);

      // Use the onComplete callback to identify the winner
      pony.animate({left: '320px'}, {
        duration: (time + 3) * 1000, // only here do we set the minimum time by adding 3
        complete: function () {
          // If this was the fastest one
          if( time === 0 ) {
            pony
              .addClass("winner")
              .append("<span>Winner!</span>"); // use a span or something, so it's easier to remove later

            // alert the gambler and enable the reset button
            if( pony.hasClass("selected") ) {
              alert("You win tons of money or something");
            } else {
              alert("You lost. Boo-hoo");
            }
            raceButton.prop("disabled", false).val("Reset");
          }
        }
      });
    });
  }

  // just reset everything yourself; no need to reload
  function reset() {
    ponies
      .stop()                  // stop any running animations
      .removeClass("selected") // clear selection
      .removeClass("winner")   // clear winner
      .css({ left: '0px' })    // move 'em back
      .children("span").remove(); // remove the label
    raceButton.val("Race!");
  }
});

---

Addendum: As Sridhar points out in the comments, you there are some issues with the code above. Both are related to the state of the race, so one way to deal with it is to extract a few functions that check the state:

function raceInProgress() {
  return ponies.filter(":animated").length > 0;
}

function raceDecided() {
  return ponies.filter(".winner").length > 0
}

function raceNotStarted() {
  return !raceInProgress() && !raceDecided();
}

Now we can make sure that the gambler can't change the selection mid-race, by simply checking raceInProgress(). Plus, it's much easier to read and use than the raw jQuery selector magic.

I've made the changes in this demo rather than bothering to rewrite my original answer.