Animated JavaScript/jQuery game

This was a code test I was asked to complete by employer, and since they rejected me, I wonder what should I have done differently.

```
var w = 7, h = 6;
var currentPlayer = 1;
var animationInProgress = 0;
var winner = 0;

var map = new Array(h);
var mapHtml = "";

for (var i = 0; i ";
for (var j = 0; j ";
}
mapHtml = mapHtml + "";
}

$("#map").html(mapHtml);

function makeTurn(col) {
if (animationInProgress == 0 && winner == 0) {
for (var i = h-1; i >= 0; i--) {
if (map[col][i] == 0) {
map[col][i] = currentPlayer;
animateTurn(col, i);
checkForWin(col, i);
currentPlayer = 3 - currentPlayer;
break;
}
}
}
}

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("");
$("#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;
}
});
}

function checkForWin(x, y) {
var delta, dx, dy, n = 0;
//horizontal win
for (i = 0; i 0; dx++, dy--) {
if (map[dx][dy] == currentPlayer) {
n++;
if (n == 4) win(currentPlayer);
} else {
n = 0;
}
}

//diagonal \ win
n = 0;
delta = Math.min(x, y);
for (dx = x-delta, dy = y-delta; dx < w && dy < h; dx++, dy++) {
if (map[dx][dy] == currentPlayer) {

I'll start with a list of observations:

-
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.

-
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.

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

-
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("");            
            $("#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("");            
                $("#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;
            }
        });            
}

-
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.

-
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.

-
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.

StackExchange Code Review Q#9156, answer score: 10