-1

I have written a program that creates a nxn matrix, then checks if the matrix is a magic square. In my method magicCheck, all elements are totaled (sum) then divided by n (the number of rows or columns) to generate a magicNumber. This magicNumber is used to compared the sum of each row, column, and diagonal individually to see if they match. The program runs fine, but when I enter numbers that should be a magic square, the program says it's not. Is there an error in the order of my code, or have I missed something in the math?

import java.util.*;

public class MagicSquare 
{
static int row, col, n, rows, columns, listSize;
static Scanner console = new Scanner (System.in);

public static void createArithmeticSeq(int [] list)
{   
    int first; 
    int diff;
    //prompt user for first and diff to create list
    System.out.println("Choose the first number and diff to create" + 
    " an arithmetic sequence.");
    System.out.println("Enter first and diff : ");
    first = console.nextInt();
    diff  = console.nextInt();
    //process to create list of n*n elements 
    for (int i=0; i<listSize; i++)
    {
        list[i]=first+i*diff;
    }
}

public static void matricize (int [] list, int [][] matrix)
{
System.out.println("Matricize arithmetic sequence:");
    int i = 0;
//loop through each row
    for (row=0; row<matrix.length; row++)
    {
    //loop through each column
        for (col=0; col<matrix[row].length; col++)
        {
        //populate matrix with values from list
        matrix[row][col] = list[i++];
        }
     }
}
public static void printMatrix(int [][] matrix)
{

    for (row=0; row < matrix.length; row++)
    {
        for (col=0; col < matrix[row].length; col++)
            System.out.printf("%2d" + " ", matrix[row][col]);

        System.out.println("\n");
    }
}

public static void reverseDiagonal(int [] [] matrix)
{ 
System.out.println("Reverse the diagonals:");
    int temp;
    for (row=0; row<matrix.length / 2; row++)
    {
        temp = matrix[row][row];
        matrix[row][row] = 
            matrix[matrix.length - 1 - row] [matrix.length - 1 - row];
        matrix[matrix.length - 1 - row][matrix.length - 1 - row] = temp;
    }
    for (row=0; row<matrix.length / 2; row++)
    {
        temp = matrix[row][matrix.length - 1 - row];
        matrix[row][matrix.length - 1 - row] = 
            matrix[matrix.length - 1 - row][row];
        matrix[matrix.length - 1 - row][row] = temp;
    }
}

public static void magicCheck(int [] list, int [] [] matrix)
{
System.out.println("Is the matrix a magic square?");
    int sum=0, sumRow=0, sumCol=0, sumDiag1=0, sumDiag2=0, magicNumber=0;
    for(int i=0; i<listSize; i++)
    {
        sum = sum + list[i];    
        magicNumber = (sum/n);

    for(row=0; row<matrix.length; row++)
    {
            //sum each row, then compare to magicNumber
        for(col=0; col<matrix[row].length; col++)
        sumRow = sumRow + matrix[row][col];
        while (sumRow == magicNumber)
        {
            for(col=0; col<matrix.length; col++)
            {
                for(row=0; row<matrix[col].length; row++)
                {
                sumCol = sumCol + matrix[row][col];
                    while (sumCol == magicNumber)
                    {
                    for (int row=0; row<matrix.length; row++)
                        {
                        sumDiag1 += matrix[row][row];
                        }
                        while (sumDiag1 == magicNumber)
                        {
                        for (int row=n-1; row>=0; row--)
                        sumDiag2 += matrix[row][row];
                            while(sumDiag2 == magicNumber)
                            System.out.println("It is a magic square.");
                            System.out.println("The magic number is " + magicNumber + ".");
                        }
                    }
                }
            }
        }
    }
    }
            System.out.println("It is not a magic square.");

}
public static void main (String [] args)
{
    //prompt user for array size
    System.out.println("Enter size of array (in form nxn), n:");
    n = console.nextInt();
    rows = n;
    columns = n;
    listSize= (n*n);    
    int [] list = new int [listSize];
    createArithmeticSeq (list);
    int [] [] matrix = new int [rows] [columns];
    matricize(list, matrix);
    printMatrix(matrix);
    System.out.print("\n");
    reverseDiagonal(matrix);
    printMatrix(matrix);
    magicCheck(list, matrix);   
}

}
false
  • 10,264
  • 13
  • 101
  • 209
woollyMammoth
  • 105
  • 1
  • 2
  • 7
  • 1
    give an example input/output as well as what you would expect it to output – Daniel Jun 10 '13 at 21:42
  • also, there's no braces for *for(col=0; col – Daniel Jun 10 '13 at 21:44
  • 2
    @Daniel What's unclear about that? It's hardly something to fret over. – Bernhard Barker Jun 10 '13 at 21:47
  • Your code is long and hard to inspect. You should find out what is causing the error and remove the parts that are important. – OptimusCrime Jun 10 '13 at 21:51
  • 1
    Well, the first problem is those `while` loops. They are either never entered or never exited. Secondly, there are way too many nested `for` loops. I don't think you need to ever go beyond two levels of nesting for this problem. – trutheality Jun 10 '13 at 21:57
  • @Dukeling I assume you're talking about the second part? Ok they don't *have* to, but I've seen errors because people didn't realize they meant to stick more than just the next line in an if/loop. If we're trying to debug, we need to be sure what the OP meant and since the formatting was (for me) unclear, I wanted to draw attention to it to make sure that wasn't the issue – Daniel Jun 10 '13 at 21:59
  • 2
    @Daniel sure, the indentation here is broken, but that doesn't mean that the right fix is to put in more braces. The right fix is to indent stuff clearly. It's just as easy to make mistakes because of poor indentation, even if you put braces everywhere where they're optional. – trutheality Jun 10 '13 at 22:03
  • @trutheality fair enough, I'll be more clear about that next time – Daniel Jun 10 '13 at 22:08
  • I'll definitely clean it up when I'm done. – woollyMammoth Jun 10 '13 at 22:15
  • As far as input/output goes, when I create a 4x4 matrix with first=2 and diff=2, this is what I get: Enter size of array (in form nxn), n: 4 Choose the first number and diff to create an arithmetic sequence. Enter first and diff : 2 2 Matricize arithmetic sequence: 2 4 6 8 10 12 14 16 18 20 22 24 26 28 30 32 Reverse the diagonals: 32 4 6 26 10 22 20 16 18 14 12 24 8 28 30 2 Is the matrix a magic square? It is not a magic square. – woollyMammoth Jun 10 '13 at 22:16
  • That's not super-clear with the formatting of the comment, but essentially, when the diagonals of the matrix are reversed, it creates a magic square. Hence, magicCheck should generate the "It is a magic square." line. – woollyMammoth Jun 10 '13 at 22:18
  • To clarify further: for the above input, magicNumber should be 68, and sumRow, sumCol, sumDiag1, and sumDiag2 should all be 68, hence generating the messages "It is a magic square." "The magic number is 68." – woollyMammoth Jun 10 '13 at 22:26
  • Have you tried `System.out.println(sumRow);`, `System.out.println(sumCol);`, `System.out.println(sumDiag1);` and `System.out.println(sumDiag2);` at strategic places? And maybe a few `System.out.println("We are here");`. These tend to help a lot with figuring out what your program is doing. Using a debugger is also a good option. – Bernhard Barker Jun 10 '13 at 22:29
  • Okay, I finally figured it out. I replaced the while loops with if, and moved the "It is not a magic square" statement to an else condition. I also cleaned up the loops a little bit and things are working smoothly now. Thanks for your help! You definitely pointed me in the right direction with the while loops @trutheality. – woollyMammoth Jun 10 '13 at 22:35

1 Answers1

1

This is not really an answer (a bit long for a comment), but I suggest you go about it this way instead:

// returns 'null' if not a magic square, otherwise returns the magic number
public static Integer getMagicNumber(int[] list, int[][] matrix)
{
  ...
  for (int i = 0; i < listSize; i++)
  {
    ...
  }
  for (row = 0; row < matrix.length; row++)
  {
    //sum each row, then compare to magicNumber
    for(col = 0; col < matrix[row].length; col++)
      sumRow = sumRow + matrix[row][col];
    if (sumRow != magicNumber)
      return null;
  }
  for (col = 0; col < matrix.length; col++)
  {
    //sum each column, then compare to magicNumber
    ...
  }
  ...
  return magicNumber;
}

Then in main you do:

Integer magicNumber = getMagic(...);
if (magicNumber == null)
  System.out.println("It is not a magic square.");
else
{
  System.out.println("It is a magic square.");
  System.out.println("The magic number is " + magicNumber + ".");
}

I'm sure someone will have a problem with having Integer taking on a value of null, but this is just one way to do things.

I didn't want to rewrite all your code for you, I'm sure you can figure it out from there.

It keeps the nested blocks to a minimum, which makes for much more readable and less error-prone code.

Bernhard Barker
  • 54,589
  • 14
  • 104
  • 138
  • Thank you. That's a useful suggestion. I'm still very new to Java, so I sometimes do things in a way that's definitely clunky and inefficient. This was one of those times. – woollyMammoth Jun 10 '13 at 23:47