Animated JavaScript/jQuery game

I'll start with a list of observations:

1. Hardly any comments. There are several places where it would be nice if you added some comments to explain how the main logic works for someone who has not seen the code before.

2. A bunch of global variables and ones using very generic names - lots of opportunities for clashes. I'd suggest either using a function closure to isolate your variables or make them properties of one global object or eliminate the need for them.

3. There is NO need for a selector like this: "#map #col2". It should just be "#col2".

4. A bunch of places where you're evaluating the same selector more than once. For example, in the animateTurn function, you evaluate $("#map .next-ball") multiple times when you could just chain. This:

   $("#map .next-ball").removeClass("player"+(3-currentPlayer));
   $("#map .next-ball").addClass("player"+currentPlayer);
   

   could be changed to this:

   $("#map .next-ball").removeClass("player"+(3-currentPlayer))
       .addClass("player"+currentPlayer);
   

   Or, in the whole function this:

   function animateTurn(col, row) {        
       animationInProgress = 1;    
       $("#map #col"+col+" .next-ball").addClass("animation-in-progress");
   
       $("#map #col"+col+" .next-ball").animate({
           top: '+='+(52*(row+1)-1)
       }, {
           duration: Math.round((row+1)*500/h),
           complete: function() {
               $("#map #col"+col+" div:nth-child("+(row+2)+")").html("<span class='ball player"+(3-currentPlayer)+"'></span>");            
               $("#map #col"+col+" .next-ball").css("top", "0px");            
               $("#map .animation-in-progress").removeClass("animation-in-progress");
               $("#map .next-ball").removeClass("player"+(3-currentPlayer));
               $("#map .next-ball").addClass("player"+currentPlayer);
               animationInProgress = 0;
           }
       });            
   }
   

   could be changed to this to use chaining and avoid re-evaluating the same selector multiple times:

   function animateTurn(col, row) {        
       animationInProgress = 1;    
       $("#map #col"+col+" .next-ball").addClass("animation-in-progress")    
           .animate({top: '+='+(52*(row+1)-1)}, {
               duration: Math.round((row+1)*500/h),
               complete: function() {
                   $("#col"+col+" div:nth-child("+(row+2)+")").html("<span class='ball player"+(3-currentPlayer)+"'></span>");            
                   $("#col"+col+" .next-ball").css("top", "0px");            
                   $("#map .animation-in-progress").removeClass("animation-in-progress");
                   $("#map .next-ball").removeClass("player"+(3-currentPlayer))
                       .addClass("player"+currentPlayer);
                   animationInProgress = 0;
               }
           });            
   }
   

5. I generally find it cleaner to use eventListeners rather than onclick handlers because you don't have to worry about multiple listeners stomping on each other.

6. You have a bunch of constants in the code that aren't very self documenting. There's a 500 and a 52 which I have no idea where they came from or when they would have to be changed if the game was modified.

7. Is there a reason that the HTML for the game is generated via javascript rather than specified separately? Usually, it's advantageous for the presentation of the app to be separate from the code for the app and for the code to adapt to whatever the presentation is. This allows separate people to work on the visual aspects of the app vs. the logic of the app and allows you to change one without affecting the other.

In addition to jfriend00's suggestions:

• You should look into the separation of model and view (MVC), which would give the code much more structure and make it easier to read IMHO.
• I would have used a <table> for the grid.
• Extending on jfriend00's point 5: A single click event handler for the whole grid instead of one for each column would make sense.
• Finally a bit more polish for the GUI would have gone a long way, especially in one aspect: The winner shouldn't be announced until the animation has completed.

EDIT:

One more point: Depending on the requirements of the job, it may make sense to write this without using jQuery (or any another framework) since this simple example hardly has any stumbling stones that require it. That way you can demonstrate that the actually understand JavaScript and the DOM, plus you could use some current technologies such as CSS 3 animations.

I concur with jfriend00 and RoToRa, and would add the following.

A style point: single var declarations are popular in JavaScript. I don't think there is anything wrong with multiple var statements, and there are even arguments for a return to this style (easier to refactor). My concern would be some people see multiple var statements as "noobish", and your potential employer may be "some people".

var w = 7,
    h = 6,
    currentPlayer = 1,
    ...
    mapHtml = "";

Prefer the array literal to new Array()

var map = [];

I agree with previous answers on putting the html for the game in the html file directly, then using JavaScript to size the game from that. So I would remove the html construction code, and set w, h (renamed) like this:

var cols = $('.cols').length,
    rows = $('.rows').length;

where the html was either a table or something like:

<div class="row">
  <div class="col"></div>
  ...    
  <div class="col"></div>
</div>
...

Function checkForWin(x, y) looks ripe for breaking up into four functions. I suspect that some of the common code in the for loops can be factored out into one function as well.

I would make checkForWin look something like this:

function checkForWin(x, y) {
    if (isHorizontalWin(x, y) ||
        isVerticalWin(x, y) ||
        isDiagonalToTopRightWin(x, y) ||
        isDiagonalToBottomRightWin(x, y)) {
            win(currentPlayer);
    }
}

These function names take your comments from the win function and embed them in the code.

Polluting the window namespace is a common but easily avoidable bad practice. There are quite a few ways of avoiding this, see this JS Namespacing article for an overview.

I would suggest an Immediately-invoked Function Expression, which is a fancy name for a self-executing anonymous function (although they do not have to be anonymous).

Wrap your code in this:

(function(){ /* your existing code goes here */})();

and you are no longer polluting the window namespace.