4
\$\begingroup\$

I'm a newbie in javascript. Could you please say what is wrong in my code? How can i improve it? Maybe something is wrong with variables or methods naming. Maybe something is wrong with common structure. My program generates magic square by order. I decided that it is good to create one main function and one class.

// *****************************
// * DEFINE MAGIC SQUARE CLASS *
// *****************************

function MagicSquare(order, startCountFrom) {
    this.order = order;
    this.startCountFrom = startCountFrom;
    this.magicSum = 0;

    this.matrix = [];
    this.initializeMatrix();
}

MagicSquare.prototype.initializeMatrix = function() {
    for (var i = 0; i < this.order; i++) {
        this.matrix.push([]);
    }
    for (var i = 0; i < this.order; i++) {
        for (var j = 0; j < this.order; j++) {
            this.matrix[i][j] = null;
        }
    }
}

MagicSquare.prototype.generate = function() {
    if (this.order % 4 === 0) {
        this.generateDoubleEvenOrderMagicSquare();
    } else if (this.order % 2 === 0) {
        this.generateSingleEvenOrderMagicSquare();
    } else {
        this.generateOddOrderMagicSquare(this.matrix, 0, 0, this.order, this.startCountFrom);
    }   
}

MagicSquare.prototype.generateDoubleEvenOrderMagicSquare = function() {
    var unusedNumbers = [];
    var counter = this.startCountFrom;

    // put numbers to the right area
    for (var i = 0; i < this.order; i++) {
        for (var j = 0; j < this.order; j++) {
            if (this.areCellIndexesFromTheRightArea(i, j)) {
                this.matrix[i][j] = counter;
            } else {
                unusedNumbers.push(counter);
            }
            counter++;
        }
    }

    // put unused numbers to the wrong area
    var indexI = unusedNumbers.length - 1;
    for (var i = 0; i < this.order; i++) {
        for (var j = 0; j < this.order; j++) {
            if (this.matrix[i][j] === null) {
                this.matrix[i][j] = unusedNumbers[indexI--];
            }
        }
    }
}

MagicSquare.prototype.areCellIndexesFromTheRightArea = function(indexI, indexJ) {
    var oneQuarterOrder = Math.floor(this.order / 4);
    var threeQuartersOrder = 3 * oneQuarterOrder;

    var topIRule = (indexI >= 0) && (indexI <= oneQuarterOrder - 1);
    var bottomIRule = (indexI >= threeQuartersOrder) && (indexI <= this.order - 1);
    var middleIRule = (indexI >= oneQuarterOrder) && (indexI <= threeQuartersOrder - 1);

    var leftJRule = (indexJ >= 0) && (indexJ <= oneQuarterOrder - 1);
    var rightJRule = (indexJ >= threeQuartersOrder) && (indexJ <= this.order - 1);
    var middleJRule = (indexJ >= oneQuarterOrder) && (indexJ <= threeQuartersOrder - 1);

    var topLeftAreaCase = topIRule && leftJRule;
    var topRightAreaCase = topIRule && rightJRule;
    var middleAreaCase = middleIRule && middleJRule;
    var bottomLeftAreaCase = bottomIRule && leftJRule;
    var bottomRightAreaCase = bottomIRule && rightJRule;

    return topLeftAreaCase || topRightAreaCase || middleAreaCase || bottomLeftAreaCase || bottomRightAreaCase;
}


MagicSquare.prototype.generateSingleEvenOrderMagicSquare = function() {
    this.createABCDSquares(this.startCountFrom);
    this.swapTwoAreas();
}

MagicSquare.prototype.createABCDSquares = function(startNumber) {
    var size = this.order / 2;
    var oneQuarterOfOrderSquare = (this.order * this.order) / 4;
    var startCountFrom = startNumber;

    // create A, B, C, D squares in cycle
    var alpha = 0;
    while (alpha <= 3 * Math.PI / 2) {
        this.generateOddOrderMagicSquare(this.matrix, Math.floor(Math.abs(Math.sin(alpha)) * (this.order / 2)), 
            Math.floor(Math.abs(Math.sin(alpha + Math.floor(alpha / Math.PI) * (Math.PI / 2))) * (this.order / 2)), 
            size, startCountFrom);
        alpha += Math.PI / 2;
        startCountFrom += oneQuarterOfOrderSquare;
    }
}

MagicSquare.prototype.swapTwoMatrixElements = function(matrix, indexIFirst, indexJFirst, indexISecond, indexJSecond) {
    var temp = matrix[indexIFirst][indexJFirst];
    matrix[indexIFirst][indexJFirst] = matrix[indexISecond][indexJSecond];
    matrix[indexISecond][indexJSecond] = temp;
}

MagicSquare.prototype.swapTwoAreas = function() {
    var d = Math.floor(this.order / 4);
    var x = d - 1;
    var halfOfOrder = this.order / 2;

    for (var i = 0; i < halfOfOrder; i++) {
        var offset = i === d ? 1 : 0;
        // left area
        for (var j = offset; j < d + offset; j++) {
            this.swapTwoMatrixElements(this.matrix, i, j, i + halfOfOrder, j);
        }
        // right area
        for (var j = this.order - x; j < this.order; j++) {
            this.swapTwoMatrixElements(this.matrix, i, j, i + halfOfOrder, j);
        }
    }
}


MagicSquare.prototype.generateOddOrderMagicSquare = function(matrix, indexIStart, indexJStart, size, startNumber) {
    var counter = startNumber;
    var indexI = indexIStart;
    var indexJ = Math.floor(size / 2) + indexJStart;

    // set one to the top middle cell 
    matrix[indexI][indexJ] = counter++;

    // fill other cells
    while (counter < size * size + startNumber) {
        if (indexI === indexIStart && indexJ === indexJStart + size - 1) {
            indexI++;
        } else if (indexI === indexIStart && indexJ < indexJStart + size - 1) {
            indexI = indexIStart + size - 1;
            indexJ++;
        } else if (indexI > indexIStart && indexJ === indexJStart + size - 1) {
            indexI--;
            indexJ = indexJStart;
        } else if (matrix[indexI - 1][indexJ + 1] !== null) {
            indexI++;
        } else {
            indexI--;
            indexJ++;
        }
        matrix[indexI][indexJ] = counter++;
    }
}

MagicSquare.prototype.isMagic = function() {
    this.magicSum = this.order * (this.order * this.order + 2 * this.startCountFrom - 1) / 2;

    var horizontalSums = this.createEmptyArray(this.order);
    var verticalSums = this.createEmptyArray(this.order);
    var mainDiagonalSum = 0, minorDiagonalSum = 0;
    for (var i = 0; i < this.order; i++) {
        for (var j = 0; j < this.order; j++) {
            horizontalSums[i] += this.matrix[i][j];
            verticalSums[j] += this.matrix[i][j];
            if (i === j) mainDiagonalSum += this.matrix[i][j];
            if (i + j === this.order - 1) minorDiagonalSum += this.matrix[i][j];
        }
    }

    var validSumsNumber = 0;
    for (var i = 0; i < this.order; i++) {
        if (horizontalSums[i] === this.getMagicSum() && verticalSums[i] === this.getMagicSum()) validSumsNumber += 2;
    }
    if (mainDiagonalSum === this.getMagicSum() && minorDiagonalSum === this.getMagicSum()) validSumsNumber += 2;
    return validSumsNumber === 2 * (this.order + 1);
}

MagicSquare.prototype.createHtmlTable = function() {
    var htmlTable = "<table>";
    for (var i = 0; i < this.order; i++) {
        htmlTable += "<tr>";
        for (var j = 0; j < this.order; j++) {
            htmlTable += "<td>" + this.matrix[i][j] + "</td>";
        }
        htmlTable += "</tr>";
    }
    htmlTable += "</table>";
    return htmlTable;
}

MagicSquare.prototype.createEmptyArray = function(size) {
    var array = [];
    for (var i = 0; i < size; i++) array[i] = 0;
    return array;
}

MagicSquare.prototype.getMagicSum = function() {
    return this.magicSum;
}


// ===========================
// GLOBAL FUNCTION DEFINITIONS
// ===========================

// test method
var checkValidity = function() {
    var counter = 0;
    for (var i = 3; i < 23; i++) {
        for (var j = -9; j < 11; j++) {
            var testMagicSquare = new MagicSquare(i, j);
            testMagicSquare.generate();
            if (testMagicSquare.isMagic()) counter++;
        }
    }
    return counter === 400;
}

// main method
var generateMagicSquare = function() {
    var time = performance.now();

    var orderValue = document.getElementById("order").value;
    if (!orderValue.match(/^[0-9]+$/)) {
        alert("Order should be a positive number.");
        throw new Error("Order should be a positive number.");
    }

    var order = parseInt(orderValue);
    if (order < 3) {
        alert("Order should be more than or equal 3. The magic square of order 2 does not exist.");
        throw new Error("Order should be more than or equal 3.");
    }

    var startCountFromValue = document.getElementById("start-count-from").value;
    if (!startCountFromValue.match(/^-?[0-9]+$/)) {
        alert("Start from field value should be an integer number.");
        throw new Error("Start from field value should be an integer number.");
    }
    var startCountFrom = parseInt(startCountFromValue);

    var magicSquare = new MagicSquare(order, startCountFrom);
    magicSquare.generate();

    // print
    document.getElementById("output").innerHTML = magicSquare.createHtmlTable();
    document.getElementById("is-magic").innerHTML = "is magic: " + magicSquare.isMagic();
    document.getElementById("magic-sum").innerHTML = "magic sum: ";
    document.getElementById("magic-sum").innerHTML += magicSquare.isMagic() ? magicSquare.getMagicSum() : "undefined";
    document.getElementById("validity-check").innerHTML = "validity check: <font color=red>failure</font>";
    if (checkValidity()) {
        document.getElementById("validity-check").innerHTML = "validity check: <font color=green>success</font>";
    }
    var timeExecutionInSeconds = Math.round(((performance.now() - time) / 1000) * 1000) / 1000;
    document.getElementById("time-execution").innerHTML = "generated in " + timeExecutionInSeconds + " sec";    
}
\$\endgroup\$
4
\$\begingroup\$

Not bad for a newbie! Here are some suggestions:

Structure

There are a few problems with the API for the MagicSquare class:

  • It's unclear what startCountFrom means.
  • In order to use a MagicSquare you have to construct one and immediately call its generate method, which isn't obvious. I expected that constructing a MagicSquare would give me a valid magic square.
  • A MagicSquare, once generated, should be a valid magic square. Therefore it's confusing that MagicSquare has a method called isMagic (actually, the method exists only for testing purposes).
  • getMagicSum returns 0 unless isMagic is called first.
  • Creating an HTML table isn't a key functionality of a magic square, so having a createHtmlTable method is a violation of the Single Responsibility Principle.
  • The value of the order argument isn't checked.

I would:

  • Add a comment before the MagicSquare constructor to explain its usage, for example: /* Create a magic square of size 'order' using the numbers 'startCountFrom' to 'startCountFrom + order^2 - 1' */
  • Call generate in the constructor. This way users of MagicSquare don't need to know about this method, and it'll also prevent the incorrect usage of calling generate 0 or multiple times.
  • Calculate magicSum when a MagicSquare is constructed, not when isMagic is called.
  • Instead of MagicSquare#isMagic, have a global function called isMagicSquareCorrect(square) (remember this function is only used for testing). This means the createEmptyArray helper function should also be global.
  • Similarly, make MagicSquare#createHtmlTable a global function.
  • Throw in the constructor if order < 3.

Note this change means that code external to MagicSquare must be able to know that square's order and matrix elements. You can acheive this without sacrificing encapsulation by adding a MagicSquare#getOrder method and a MagicSquare#getAt(x, y) method (to get the number at a particular cell).

Also, I recommend putting the public functions first (functions called by code external to MagicSquare) and then the private ones. You may want to follow the convention of prefixing the names of private fields and functions with an underscore.

Naming

Instead of using i and j, I recommend using y and x whenever they're used as matrix indexes, because that's the standard notation for vertical and horizonal coordinates.

areCellIndexesFromTheRightArea

var oneQuarterOrder = Math.floor(this.order / 4);

Don't call Math.floor if the argument is known to be an integer. I also suggest renaming this variable to quarterSize.

var topIRule = (indexI >= 0) && (indexI <= oneQuarterOrder - 1);
var bottomIRule = (indexI >= threeQuartersOrder) && (indexI <= this.order - 1);
var middleIRule = (indexI >= oneQuarterOrder) && (indexI <= threeQuartersOrder - 1);
  • Prefer 0 <= x && x <= 4 over x >= 0 && x <= 4, because it's closer to the conventional mathematical notation 0 <= x <= 4 and therefore more readable.
  • Prefer indexI < this.order over indexI <= this.order - 1.
  • indexI is known to be between 0 and this.order; there's no need to check it.
  • Reorder the lines top-middle-bottom, so it's more natural.
  • The names of these variables can be shortened to isTop, isBottom, isMiddle. The next three can be named isLeft, isRight and isCenter.

Also, notice that you only care about whether the position is at the center or not. If it's not at the center, it doesn't matter if it's on the left or on the right. Therefore you only need isCenter (and isMiddle), and you can get rid of isLeft and isRight (and isTop and isBottom).

Suggested solution:

MagicSquare.prototype.areCellIndexesFromTheRightArea = function(y, x) {
    var quarterSize = this.order / 4;

    var isMiddle = quarterSize <= y && y < 3 * quarterSize;
    var isCenter = quarterSize <= x && x < 3 * quarterSize;

    return (isMiddle && isCenter) || (!isMiddle && !isCenter);
}

You can simplify the last line to this, if you think it's understandable:

return isMiddle === isCenter;

generateOddOrderMagicSquare

Rename indexIStart, indexJStart, indexI, indexJ to y0, x0, y, x.

var x = Math.floor(size / 2) + x0;

I think it's more elegant not to use Math.floor:

var x = x0 + (size - 1) / 2;

It's hard to see that the underlying logic is "go diagonally if possible, otherwise go down", because it's split to 4 different cases to handle all possible ways of wrapping around the square. It's simpler to first go diagonally (decrement y, increment x), and then check if any of x and y needs wrapping.

Suggested solution:

MagicSquare.prototype.generateOddOrderMagicSquare = function(matrix, y0, x0, size, startNumber) {
    // use the Siamese method

    var counter = startNumber;
    var y = y0;
    var x = x0 + (size - 1) / 2;

    // start at the top middle
    matrix[y][x] = counter++;

    // fill other cells
    while (counter < size * size + startNumber) {
        // go diagonally
        var newY = y - 1;
        var newX = x + 1;

        // wrap around
        if(newY < y0)
            newY += size;
        if(newX === x0 + size)
            newX -= size;

        if(matrix[newY][newX] !== null) {
            // go down instead
            var newY = y + 1;
            var newX = x;
        }

        y = newY;
        x = newX;
        matrix[y][x] = counter++;
    }
}

(Notice that the check of whether the new cell is filled happens in all cases now).

createABCDSquares

// create A, B, C, D squares in cycle
var alpha = 0;
while (alpha <= 3 * Math.PI / 2) {
    this.generateOddOrderMagicSquare(this.matrix, Math.floor(Math.abs(Math.sin(alpha)) * (this.order / 2)), 
        Math.floor(Math.abs(Math.sin(alpha + Math.floor(alpha / Math.PI) * (Math.PI / 2))) * (this.order / 2)), 
        size, startCountFrom);
    alpha += Math.PI / 2;
    startCountFrom += oneQuarterOfOrderSquare;
}

The trigonometry here looks complicated. I think the code will be simpler if you replace this loop with 4 calls to this.generateOddOrderMagicSquare, eliminating alpha and calls to Math functions.

Possible simplification

Instead of having each of the three methods for generating magic squares use startCountFrom, considering using 0 as the initial value in all three. Then change generate to add startCountFrom to all matrix elements after calling the appropriate generation function.

\$\endgroup\$
  • \$\begingroup\$ Thank you. I applied your suggestions and the code started to look much better. \$\endgroup\$ – hrrmsn Nov 28 '15 at 16:33

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.