Magic square creator

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 = 0) && (indexI = threeQuartersOrder) && (indexI = oneQuarterOrder) && (indexI = 0) && (indexJ = threeQuartersOrder) && (indexJ = oneQuarterOrder) && (indexJ 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 ";
for (var i = 0; i ";
for (var j = 0; j " + this.matrix[i][j] + "";
}
htmlTable += "";
}
htmlTable += "";
return htmlTable;
}

MagicSquare.prototype.createEmptyArray = function(size) {
var array = [];
for (var i = 0; i failure";
if (checkValidity()) {
document.getElementById("validity-check").innerHTML = "validity check: success";
}
var timeExecutionInSeconds = Math.round(((performance.now() - time) / 1000) * 1000) / 1000;
docum

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




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 = threeQuartersOrder) && (indexI = oneQuarterOrder) && (indexI <= threeQuartersOrder - 1);

• Prefer 0 = 0 && x

• Prefer indexI






• 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;

StackExchange Code Review Q#110093, answer score: 4