2
\$\begingroup\$

I am currently working on an unfinished project about Chess, it will (at some point) be able to help me to make some chess variations just by pasting the FEN position while I play online.

But before continuing, my code must have good structure or it will become a nightmare to fix it later. So now that I have some basics, it would be great to have it reviewed:

//Note: the code uses jQuery

function writeBoard() {
    var i, j, str, bol;

    str = "<table id=\"chessboard\" cellpadding=\"0\" cellspacing=\"0\"><tbody>";
    for (i = 9; --i;) {
        str += "<tr>";
        for (j = 0; j < 8; j++) {
            bol = !bol;
            str += "<td class=\"" + (bol ? "ws" : "bs") + "\" id=\"" + (AbcLabels[j] + "" + i) + "\"></td>";
        }
        bol = !bol;
        str += "</tr>";
    }
    str += "</tbody></table>";

    $("body").append(str);
}

function setFEN(fen) {
    var i, j, len, arr, tmp, tmp2, tmp3;

    arr = (fen ? fen : DefaultFen).split(" ")[0].split("/");
    for (i = 0; i < 8; i++) {
        tmp = i * 8;
        for (j = 0, len = arr[i].length; j < len; j++) {
            tmp2 = arr[i].charAt(j);
            if (tmp2 * 1) {
                tmp += tmp2 * 1;
            } else {
                tmp3 = tmp2.toLowerCase();
                ChessBoard[i][tmp % 8] = PiecesNames.indexOf(tmp3) * (tmp2 != tmp3 ? 1 : -1);
                tmp++;
            }
        }
    }

    refreshBoard();
}

function refreshBoard() {
    var i, j, emt, fen, tmp, tmp2, tmp3, tmp4;

    fen = "";
    for (i = 9; --i;) {
        emt = 0;
        for (j = 0; j < 8; j++) {
            tmp = Math.abs(i - 8);
            tmp2 = ChessBoard[tmp][j] || 0;
            if (tmp2) {
                if (emt) {
                    fen += "" + emt;
                    emt = 0;
                }

                tmp3 = $("#" + AbcLabels[j] + "" + i);
                if (tmp2 < 0) {
                    tmp4 = PiecesNames.charAt(tmp2 * -1);
                    tmp3.addClass("b" + tmp4);
                    fen += tmp4;
                } else {
                    tmp4 = PiecesNames.charAt(tmp2);
                    tmp3.addClass("w" + tmp4);
                    fen += tmp4.toUpperCase();
                }
            } else {
                emt++;
            }
        }

        if (emt) {
            fen += "" + emt;
        }
        fen += "/";
    }

    console.log(fen); //remove the last "/"
    //fen+= can enPass, to move, castle Avility, clock, etc...
    //Fen=fen
}

var I, J, Len;
var AbcLabels = ["a", "b", "c", "d", "e", "f", "g", "h"];
var PiecesNames = "*pnbrqk"; //[wp:1][wn:2][wb:3][wr:4][wq:5][wk:6]***[bp:-1][bn:-2][bb:-3][br:-4][bq:-5][bk:-6]
var DefaultFen = "rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq -";
var ChessBoard = new Array(8);
for (I = 8; I--;) {
    ChessBoard[I] = new Array(8);
}
var Fen;
var ToMove; //[0:white][1:black]
var WCastling, BCastling; //[0:none][1:short][2:long][3:both]
//var IsCheck,EnPassant;

$(function() {
    writeBoard();
    setFEN("5Rk1/1b2p2p/p2pP1p1/2rP4/2BQ2P1/6qP/PP6/1K6 b - -");
});

If you don't have a good text editor, you will find tmpX variables very hard to recognize one over the another... In my case, I just double click and they all get highlighted (using Notepad++).

\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

The first thing I noticed are the escaping orgies. Both HTML and JavaScript allow you to use either ' or " to quote strings. Don't use the same for both and you can get rid of all those ugly backslashes.

You can also get rid of the (badly-named) bol variable. You already have a loop variable so you can use j % 2 == 0 or even simpler j & 1 to determine if the number is even or odd.

\$\endgroup\$
0
1
\$\begingroup\$

Not a big java person, but I'd say rename all your variables, especially the tmp ones. When you come back in 6 months, are you really going to know what tmp2, or fen is representing? Try coming up with more descriptive names, even if the name is longer, in the long run you'll thank yourself.

\$\endgroup\$
4
  • \$\begingroup\$ every single tmpX is unimportant, they are just vars that hold a cache of something to don't write or call them twice. But renaming the others (arr,bol,str,etc) is also a possibility, 3 letter names are not very descriptive \$\endgroup\$
    – ajax333221
    Commented Aug 13, 2012 at 19:13
  • \$\begingroup\$ I agree with Jeff. tmp is fine but if you have three different temp variables a more meaningful name is usually appropriate. \$\endgroup\$ Commented Aug 13, 2012 at 19:24
  • 1
    \$\begingroup\$ @Jeff This is JavaScript not Java. Completely unrelated concepts. \$\endgroup\$
    – RoToRa
    Commented Aug 14, 2012 at 12:06
  • \$\begingroup\$ Why is having descriptive variable names unrelated? Descriptive variable names are good for cleanliness in any language. \$\endgroup\$ Commented Aug 14, 2012 at 15:28
1
\$\begingroup\$

First obvious point: In JavaScript it's custom to give variables names starting with lower case letters.

One thing I would do is remove the "writing" of the board from the JavaScript and use a static HTML table instead.

You could remove both the class and the id from the table cells. The checkered color can easily be created with CSS, e.g.

#chessboard tr:nth-child(2n) > td:nth-child(2n+1), #chessboard tr:nth-child(2n+1) > td:nth-child(2n) {
    background-color: black;
}​

and the cells can be accessed using the rows and cells properties of the table DOM.

Prefilling the variable tmp (the file) with i * 8 doesn't mnake much sense, considering you do tmp % 8 anyway.

I'm not a big fan of displaying the pieces with CSS alone leaving the table practically empty. You should consider filling the cells with the appropriate Unicode characters and use the usual CSS text replacement techniques, if you want to display nicer images.

Is there a any special reason you are writing the setup to a global variable (ChessBoard) before filling the table? Global variables is something one always should avoid when programming. And splitting up both steps just requires you to run though the loops twice. The only advantage I see in spitting the two steps up is the separation of concerns (model, view), which doesn't seem to be a priority here.

Speaking of which, the "encoding" of the pieces in the ChessBoard as positive and negative numbers is quite cryptic. If you want to reuse that information, you may want to use something more readable, such as an object. Or at least document in a comment what you are doing.

The identification of digits by multiplication with one isn't very common, and should also be commented. Using the unitary plus operator is more common, and explicitly testing with ìsNaN would make it more readable:

if ( !isNaN(+tmp2) ) {

Finally fen ? fen : DefaultFen can be shorted to fen || DefaultFen

Here's how I would do it: http://jsfiddle.net/xL9B8/4/

\$\endgroup\$
3
  • \$\begingroup\$ +1, you have some good points there. I will post the ones I disagree: vars should start with lower case Globals should not. reason to have everything in ChessBoard because I am planning to check legal moves, and getting values from the DOM is expensive and slow. \$\endgroup\$
    – ajax333221
    Commented Aug 14, 2012 at 15:25
  • \$\begingroup\$ Well, you shouldn't use globals in the first place :-) \$\endgroup\$
    – RoToRa
    Commented Aug 15, 2012 at 14:03
  • \$\begingroup\$ 8+ years and I cringe looking back at my code haha, true, I don't expose globals anymore, my "library" only exposes one global, like jQuery. Had no idea on any design patterns lol. \$\endgroup\$
    – ajax333221
    Commented Jan 19, 2021 at 5:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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