Magic square test

Question

A magic square adds up the values in each vertical, horizontal, and diagonal row to the same value. In this case, that value is 34. I have changed different indices values and the program returns false. It seems to work just fine. But I'm just learning 2d arrays. Please let me know if you see something wrong, much appreciated.

public class Blank {
    public static void main(String[] args) {
    int [][] square = {

      {16, 3, 2, 13},
      {5, 10, 11, 8},
      {9, 6, 7, 12 },
      {4, 15, 14, 1}
      }; 
   System.out.println("Is magic square: " + magicSquare(square));   
 }
   private static boolean magicSquare(int[][] square){

   //calculate the sum of the first row and assign it to n
       int n = sumOfRow(square[0]);

       for (int[] row : square)
       {
          int sum = sumOfRow(row);        
          if (sum != n)
          return false;   
       }

       int sum = 0;
       for (int i = 0; i < square.length; i++)
       {
          sum += square[i][i];
       }
       if (sum != n)
          return false;

       sum = 0;
       for (int i = 0; i < square.length; i++)
       {
          sum += square[i][square.length - 1 - i];
       }
       if (sum != n)
          return false;
       return true;
    } 

   //returns the sum of the elements in the row
   private static int sumOfRow(int[] row){
      int sum = 0;
      for(int el : row){
         sum += el;
      }
      return sum;
   }
}

Answer 1

There are a couple of bugs in the current implementation.

Empty array (no rows)

First of all

int n = sumOfRow(square[0]);

is problematic, because it will throw an exception when the array is empty, and this wasn't tested before. You could add an early-return testing for the particular case of an empty array:

if (square.length == 0) {
    return false; // or true, depends if you consider an empty 2D array as magic
}

Is it even a square?

There is also no verification that the 2D array is in fact a square, that is to say that each row and each column have the same length. This is also problematic because the rest of the code will raise an exception in this case, instead of simply returning false. For example, consider running it with { {1, 2}, {3} }: the row test will pass but then it'll fail at testing the diagonals. A simple way to take care of it would be the following:

for (int i = 0; i < square.length; i++) {
    if (square.length != square[i].length) {
        return false;
    }
}

We know the array has at least one row, due to the previous early-return. Note that there are tricky cases: if the input is:

int [][] square = {
  {1, 2}, // or simply {}
};

There is actually one row, and 2 columns. As such, the general algorithm is to get the number of rows of the array with array.length, and then check whether each row array[i] has that same length.

The columns

The rest of code correctly tests whether rows and both diagonals have the same sum, but it doesn't consider columns. The current algorithm considers

int [][] square = {
  {1, 2, 3},
  {5, 1, 0},
  {2, 0, 4},
};

to be a magic square, when it isn't, because the sum of the columns do not match, although the sum of each row, and each each diagonal is equal to 6.

Answer 2

Naming

public class Blank {

That doesn't seem like a descriptive name. I'd have expected something more like MagicSquare or even SquareUtils.

   private static boolean magicSquare(int[][] square){

Does this magic square something? Methods names are generally verbs describing what the method does. In this case, it tells if it isMagicSquare.

   private static boolean isMagicSquare(int[][] square) {

It is common for methods with is or has names to return boolean values.

Optimizing

       int n = sumOfRow(square[0]);

You calculate the sum of the first row twice, but you don't need to do so. Consider

       final int n = sumBackwardDiagonal(square);

with

   public int sumBackwardDiagonal(int[][] square) {
       int lastColumn = square[0].length - 1;

       int sum = 0;
       for (int column = 0; column <= lastColumn; column++)
       {
           sum += square[lastColumn - column][column];
       }

       return sum;
   }

You have to do that anyway, so doing it first just saves effort.

Calculating lastColumn first ensures that we don't do extra calculations on each iteration of the loop. The compiler would probably handle that.

Making n final ensures that people won't expect it to change. You can capitalize n if you want. Constants in ALL_CAPS is a common convention.

Arguable style

It makes no functional difference in

       for (int[] row : square)
       {
          int sum = sumOfRow(row);        
          if (sum != n)
          return false;   
       }

but I'd prefer

       for (int[] row : square) {
          if (sumOfRow(row) != n) {
              return false;
          }
       }

In general, I prefer not to use the single statement form of control statements (if, for, etc.). It can cause bugs, particularly when multiple people edit the same file. I find it simplest to never use them rather than to find the narrow circumstances where it will work.

Since you only use it once, I wouldn't bother with the sum variable.

I prefer to put the { on the same line as its control statement. This also fits the common Java coding standard.

Keep it short

       if (sum != n)
          return false;
       return true;

You can just say

       return sum != n;

This pattern works any time you return true if the expression is true and false otherwise.

Answer 3

Here is an updated version of the code, it includes a check on the columns, a more descriptive class name, and have also tested it with arrays of different sizes. I appreciate the suggestions and pointing out that i was missing a check.

public class MagicSquare{
  public static void main (String[] args){
   int [][] square = {

      {16, 3, 2, 13},
      {5, 10, 11, 8},
      {9, 6, 7, 12 },
      {4, 15, 14, 1}
      }; 
   System.out.println("Is magic square: " + magicSquare(square));   
 }
   private static boolean magicSquare(int[][] square){

   //calculate the sum of the first row and assign it to n
       int n = sumOfRow(square[0]);

       // sum of rows check against n
       for (int[] row : square)
       {
          int sum = sumOfRow(row);        
          if (sum != n)
          return false;   
       }
       int sum = 0;

       //sum of colums check against n
       for(int i = 0; i < square.length; i ++){
         sum = 0;
         for(int j = 0; j < square.length; j++){
          sum += square[j][i]; 
         }
         if(sum != n){
            return false;
         }
       }

       //diagonal from top left corner to right bottom corner
        sum = 0;
       for (int i = 0; i < square.length; i++)
       {
          sum += square[i][i];
       }
       if (sum != n)
          return false;

     //diagonal from top right to bottom left corner 
       sum = 0;
       for (int i = 0; i < square.length; i++)
       {
          sum += square[i][square.length - 1 - i];
       }
       if (sum != n)
          return false;
       return true;
    } 

   //returns the sum of the elements in the row
   private static int sumOfRow(int[] row){
      int sum = 0;
      for(int el : row){
         sum += el;
      }
      return sum;
   }

}