2
\$\begingroup\$

I am nearly done developing my pong game (made in HTML5 / JavaScript), and I would like some constructive criticism on my code.

Full code

Initialization:

var play2 = false;

function init() {
//setup canvas
canvas = document.getElementById("canvas");
ctx = canvas.getContext("2d");
ctx.canvas.width = window.innerWidth;
ctx.canvas.height = window.innerHeight;
ctxW = ctx.canvas.width;
ctxH = ctx.canvas.height;
ctxWD2 = ctxW / 2;
ctxHD2 = ctxH / 2;
//var arrays
KEY = {
        UP: 38,
        DOWN: 40,
        W: 87,
        S: 83,
        D: 68,
        P: 80,
        O: 79,
        I: 73,
        SPACE: 32
}
pong = {
        ver: "v1.6beta",
        fps: 60,
        win: 10,
        pressedKeys: [],
        ntfs: [],
        pwrs: [],
        debug: true,
        paused: false,
        respawning: false,
        ball: {         
                x: null,
                y: null,
                w: 20,
                wM: 0,
                h: 20,
                hM: 0,
                s: null,
                sM: 0,
                dirX: 1,
                dirY: 1,
                c: "#FFFFFF",
        },
        pA: {
                x: 100,
                y: null,
                w: 10,
                h: 80,
                hM: 0,
                s: 8,
                sM: 0,
                c: "#FFFFFF",
        },
        pB: {
                x: null,
                y: null,
                w: 10,
                h: 80,
                hM: 0,
                s: 8,
                sM: 0,
                c: "#FFFFFF",
        },
        sA: {
                val: 0,
                x: null,
                y: 20,
                c: "#FFFFFF",
        },
        sB: {
                val: 0,
                x: null,
                y: 20,
                c: "#FFFFFF",
        },
        cmpt: {
                val: false,
                txt: null,
                txt2: "Press SPACE to Play Again",
                x: null,
                y: 20,
                c: "#FFFFFF",
        },
        pwr: {
                x: null,
                y: null,
                w: 10,
                h: 10,
                s: 5,
                c: "#FFFFFF",
                move: false,
                dir: null,
                obj: null,
                rsp: 0,
        },
        pAI: {
                pA: true,
                pB: true,
        },
        pAIOpt: {
                miss: 0,
                diff: 1200,
        },
        render: {
                ball: true,
                pA: true,
                pB: true,
                sA: true,
                sB: true,
                ntf: true,
                pwr: false,
                cmpt: false,
                paused: false,
                version: true
        }
};
//redefine sub-arrays
ball = pong.ball;
pA = pong.pA;
pB = pong.pB;
sA = pong.sA;
sB = pong.sB;
cmpt = pong.cmpt;
pwr = pong.pwr;
pAI = pong.pAI;
pAIOpt = pong.pAIOpt;
render = pong.render;
//adjust positioning according to window size
ball.s = ctxW / 250;
pA.y = (ctxH - pA.h) / 2;
pB.y = (ctxH - pB.h) / 2;
pB.x = ctxW - 100 - pB.w;
sA.x = ctxWD2 - 200;
sB.x = ctxWD2 + 200;
cmpt.x = ctxWD2;
//initialize key listeners
for (var keyCode in KEY) {
        if (KEY.hasOwnProperty(keyCode)) {
                pong.pressedKeys[KEY[keyCode]] = {
                        isDown: false,
                        wasDown: false
                };
        }
}
$(document).keydown(function(e) {
        for (var x in KEY) {
                if (KEY[x] == e.which) {
                        pong.pressedKeys[e.which].isDown = true;
                }
        }
});
$(document).keyup(function(e) {
        for (var x in KEY) {
                if (KEY[x] == e.which) {
                        pong.pressedKeys[e.which].isDown = false;
                }
        }
});

//initialize startloop
if (!play2) {
        pong.startloop = setInterval(startloop,1000/pong.fps);
}
else if (play2) {
        respawnBall();
        pong.gameloop = setInterval(gameloop,1000/pong.fps);
}
}

function startloop() {
checkResize();
ctx.clearRect(0,0,ctxW,ctxH)
ctx.font = "60px pong";
ctx.textAlign = "center";
ctx.textBaseline = "middle";
ctx.fillStyle = "#FFFFFF";
ctx.fillText("Press SPACE to Start",ctxW / 2,ctxH / 2);
if (pong.pressedKeys[KEY.SPACE].isDown && !pong.pressedKeys[KEY.SPACE].wasDown) {
        //initialize gameloop
        clearInterval(pong.startloop);
        //get the ball rolling (not really)
        respawnBall();
        pong.gameloop = setInterval(gameloop,1000/pong.fps);
}
for (var keyCode in KEY) {
        if (KEY.hasOwnProperty(keyCode)) {
                pong.pressedKeys[KEY[keyCode]].wasDown = pong.pressedKeys[KEY[keyCode]].isDown;
        }
}
}

Render function:

function renderGraphics() {
//resize if window size changes
checkResize();
//clear canvas
ctx.clearRect(0,0,ctxW,ctxH)
//draw ball
if (render.ball) {
        ctx.fillStyle = ball.c;
        ctx.fillRect(ball.x,ball.y,ball.w + ball.wM,ball.h + ball.hM);
}
//draw pA
if (render.pA) {
        ctx.fillStyle = pA.c;
        ctx.fillRect(pA.x,pA.y,pA.w,pA.h + pA.hM);
}
//draw pB
if (render.pB) {
        ctx.fillStyle = pB.c;
        ctx.fillRect(pB.x,pB.y,pB.w,pB.h + pB.hM);
}
//draw pwr
if (render.pwr) {
        ctx.fillStyle = pwr.c;
        ctx.fillRect(pwr.x,pwr.y,pwr.w,pwr.h);
}
//draw scores
ctx.font = "120px pong";
ctx.textAlign = "center";
ctx.textBaseline = "top";
if (render.sA) {
        ctx.fillStyle = sA.c;
        ctx.fillText(sA.val,sA.x,sA.y);
}
if (render.sB) {
        ctx.fillStyle = sB.c;
        ctx.fillText(sB.val,sB.x,sB.y);
}
//draw win screen
if (render.cmpt) {
        ctx.fillStyle = cmpt.c;
        ctx.fillText(cmpt.txt,cmpt.x,cmpt.y);
        ctx.font = "60px pong";
        ctx.fillText(cmpt.txt2,cmpt.x,ctxH - 100);
}
//draw pause screen
if (render.paused) {
        ctx.fillStyle = "#FFFFFF";
        ctx.fillText("Paused",ctxWD2,20);
}
//draw version number
if (render.version) {
        ctx.font = "20px pong";
        ctx.textAlign = "right";
        ctx.fillText(pong.ver,ctxW - 5,5);
}
//draw ntf
if (render.ntf) {
        pong.ntfs.forEach(function ntf(I) {
                ctx.font = I.f;
                ctx.textAlign = I.a;
                ctx.textBaseline = I.b;
                ctx.fillStyle = I.c;
                if (I.render) {
                        ctx.fillText(I.txt,I.x,I.y);
                }
        });
}
}

AI calculations:

function calculateAI() {
for (paddle in pAI) {
        var obj = pong[paddle];
        var by = ctxHD2;
        if (!pAI[paddle]) {
                return;
        }
        if (pAIOpt.miss == 0) {
                switch(rand(pAIOpt.diff)) {
                        case 0:
                                pAIOpt.miss = 30;
                                break;
                        case 1:
                                pAIOpt.miss = 60;
                                break;
                }
        }
        else if (pAIOpt.miss > 0) {
                pAIOpt.miss--;
                return;
        }
        switch(obj) {
                case pA:
                        var dir = -1;
                        break;
                case pB:
                        var dir = 1;
                        break;
        }
        if (dist(ball,obj) <= ctxW * 0.3 && ball.dirX == dir) {
                by = ball.y + (ball.h / 2) - ((obj.h + obj.hM) / 2);
        }
        if (dist(pwr,obj) <= ctxW * 0.3 && render.pwr) {
                if (ball.dirX == dir * -1 || dist(ball,obj) > ctxW * 0.2) {
                        by = pwr.y + (pwr.h / 2) - ((obj.h + obj.hM) / 2);
                }
        }
    if (by > obj.y + ((obj.h + obj.hM) / 2)) {
        obj.y += obj.s + obj.sM;
    }
    else if (by < obj.y - ((obj.h + obj.hM) / 2)) {
            obj.y -= obj.s + obj.sM;
        }
}
}
\$\endgroup\$
3
+50
\$\begingroup\$

I am not a "pro member" but can still leave a few comments. They are not too important anyway. You should move to more complicated games and will see for yourself whether you agree with me or not.

  • Please indent your code properly.
  • Do you know about JavaScript closures? I'm sure they could be helpful here.
  • Your variable names could be more explicit. What is cmpt, sM or val?. If you're really worried about .js size, minify it.
  • I realize that most game frameworks say that the origin is at some corner (eg. (0,0) is bottom left), but you could avoid to repeat yours? Never use names like elf if the origin was in the middle. For example, to check if a player won a point or not, you could write:

    if (Math.abs(ball.x) > ctxWD2) {
        winner = ball.x < 0 ? sA : sB;
        winner.val++;
    }
    
  • It feels weird to separate the respawnBall() call from the "Player 1 wins" code.

  • This is probably a bug: you probably meant break;, you don't want to exit the loop too quickly if there's an AI next. if (!pAI[paddle]) { return; }
  • You're mixing rendering logic and main logic, which is OK for pong but will bite you whenever you're making something bigger. I would prefer having a few states ("playing", "display score", ...) and having the rendering function deciding what to render than all those render.xxx = true in the code
  • The powerup animation is very nice. :)
  • Please use the ternary operator when you're making assignments:

    switch(obj) {                                                                                                            
        case pA:                                                                                                             
            var dir = -1;                                                                                                    
            break;                                                                                                           
        case pB:                                                                                                             
            var dir = 1;                                                                                                     
           break;                                                                                                           
    }
    

    could become dir = obj == pA ? -1 : 1. You can also use a if(), but the switch feels overkill.

\$\endgroup\$
  • \$\begingroup\$ Wow, great response! 1. How is it not indented properly as is? 2. No I don't, do you have a link to more info? 3. I shrunk them to make coding faster, I'll include a reference to say which is which. 4. I'm not sure what you mean. I'm not using a framework. 5. I separated it because I call it from various places. 6. I used return over break purposely. I didn't want the function to continue. 7. I know my render segment is a bit haphazard, I'll take a look at cleaning it up. 8. Thanks :) 9. I didn't even know JS had a ternary operator. I'll put that in there. \$\endgroup\$ – Ryan S. Feb 28 '12 at 14:55
  • \$\begingroup\$ Overall, thanks for all the feedback! You definitely made some good points. I will try implementing them and see how it goes. If I don't get any better answers within the next couple days, I'll award you the bounty :) \$\endgroup\$ – Ryan S. Feb 28 '12 at 15:01
  • \$\begingroup\$ RE: Indention: You haven't indented the the function bodys and 8 spaces is far too much indention. Two to four are used normally. Plus you should use some empty lines now and then. \$\endgroup\$ – RoToRa Feb 28 '12 at 15:30
  • 2
    \$\begingroup\$ RoToRa, he's saying they make the coding faster. Which could make sense, except that I'm sure he's not spending most of his time typing, but thinking. For indentation, 8 spaces is OK, but there are still problems, eg. the last two lines of calculateAI. \$\endgroup\$ – Quentin Pradet Feb 28 '12 at 15:34
  • 1
    \$\begingroup\$ @Cygal I don't indent my function bodies because that would cause pretty much all my code to be indented. It just seems annoying to me. Idk, I guess its just one of my quirks :P \$\endgroup\$ – Ryan S. Feb 28 '12 at 23:11
3
\$\begingroup\$

Here are some random thoughts:

  • The whole code seems a quite unstructured. For example, it's complete unclear why which function is in which file. And the unreadable short variable names don't help there either.
  • There is a lot of code duplication.
  • There are virtually no comments that really explain things, especially the many calculations.
\$\endgroup\$
  • \$\begingroup\$ 1. I group the functions into the different files based on what they are related to. For instance, all the powerup functions are in the powerup.js file, and all the functions I call from various places that don't specifically fall into another category, I put in the util.js file. All the functions called in the gameloop in gameloop.js, and the code that runs once when the game starts in init.js. 2. I know there is a lot in powerup.js, but how would I fix it? 3. I'll add them in once I fix some of the other issues. \$\endgroup\$ – Ryan S. Feb 28 '12 at 16:22
  • \$\begingroup\$ Re: 1. I didn't say there wasn't a logic, but that the logic isn't really noticable. Especially the "utils.js" seems like a collection of random mixture of actual utility functions such as rand and game logic functions. Re: 2. I'll post some suggestions (possibly tomorrow). \$\endgroup\$ – RoToRa Feb 28 '12 at 16:34
2
\$\begingroup\$

Your application should be defined in a scope.

like

PingPong = {}; PingPong.calculateAI= function(){...}

\$\endgroup\$
  • \$\begingroup\$ I do this for most everything in the app, except the functions. I don't see how doing that for my functions would help in any way. \$\endgroup\$ – Ryan S. Feb 28 '12 at 1:46
  • \$\begingroup\$ if you release this a plugin, it is safe to have your own scope otherwise it may conflict with some other functions. \$\endgroup\$ – Amareswar Feb 28 '12 at 1:49
  • \$\begingroup\$ I see. I will consider doing that if I ever release it as one. \$\endgroup\$ – Ryan S. Feb 28 '12 at 1:51
  • \$\begingroup\$ @ryansworld It's not only the functions. You've declared almost all variables in the global scope, too. Using a single namespace/scope allows your code the run along side other JavaScript in the same document without the danger of names to clash. \$\endgroup\$ – RoToRa Feb 28 '12 at 15:33
  • \$\begingroup\$ @RoToRa When I re-defined all the sub-objects of pong to global objects, I did that so I didn't have to type pong. before every var. If you could suggest a better way to do that I would love to hear it. (Please don't interpret that as sarcasm :P) \$\endgroup\$ – Ryan S. Feb 28 '12 at 16:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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