"Magic Squares" assignment

Question

The assignment is to take a "Magic Square" .txt file (where the horizontal lines, vertical lines and diagonal line all add up to the same number), read it and prove that they are in fact magic squares.

Is anything in this answer bad practice or just bad? I'm using a lot of Lists and for loops.

package magicSquares;

public  MagicSquaresCormac() {
    return;
}

private List<List<Integer>> getNumberList(String file) throws IOException {
    FileReader fr = new FileReader(file);
    BufferedReader br = new BufferedReader(fr);

    String line = null;
    Integer num = null;
    List<String> lineList = new ArrayList<>();
    List<String> charList;
    List<List<String>> listOfNums = new ArrayList<>();
    List<Integer> numlist;
    List<List<Integer>> numListList = new ArrayList<>();

    while ((line = br.readLine()) != null) {
        line.split("\t");
        if (!line.equals("")) {
            lineList.add(line);
        }
    }

    for (String lineFromList : lineList) {
        //    lineFromList.split(" ");
        numlist = new ArrayList<>();
        charList = Arrays.asList(lineFromList.split("\\s"));
        listOfNums.add(charList);
        for (String character : charList) {
            num = Integer.valueOf(character);
            numlist.add(num);
        }
        numListList.add(numlist);
    }

    br.close();
    return numListList;
}

private boolean horizontalLine(String file) throws IOException {
    List<List<Integer>> numListList = getNumberList(file);
    boolean isMagic = true;
    Integer lastSum = -1;
    Integer sum;

    for (List<Integer> listNum : numListList) {
        sum = 0;
        for (Integer i : listNum) {
            sum = sum + i;
        }
        if (lastSum == -1)
            lastSum = sum;

        else if (!lastSum.equals(sum)) {
            isMagic = false;
        }
    }

    return isMagic;
}

private boolean diagonalLine(String file) throws IOException {
    List<List<Integer>> numListList = getNumberList(file);
    boolean isMagic = true;
    Integer sum;
    Integer sumDiagonal = 0;

    for (int j = 0; j < numListList.size(); j++) {
        sumDiagonal = sumDiagonal + numListList.get(j).get(j);
    }

    sum = sumDiagonal;

    if (!sum.equals(260)) {
        isMagic = false;
    }
    return isMagic;
}

private boolean verticalLine(String file) throws IOException {
    List<List<Integer>> numListList = getNumberList(file);
    boolean isMagic = true;
    Integer lastSum = -1;
    Integer sum;
    List<Integer> tempList;
    List<List<Integer>> verticalLists = new ArrayList<>();

    for (int k = 0; k < numListList.size(); k++) {
        tempList = new ArrayList<>();
        for (int i = 0; i < numListList.size(); i++) {
            tempList.add(numListList.get(i).get(k));
        }
        verticalLists.add(tempList);
    }

    for (List<Integer> listNum : verticalLists) {
        sum = 0;
        for (Integer i : listNum) {
            sum = sum + i;
        }
        if (lastSum == -1)
            lastSum = sum;

        else if (!lastSum.equals(sum)) {
           System.out.println((lastSum != sum) + "really?");
            isMagic = false;
        }
    }
    return isMagic;
}

public static void main(String[] args) throws IOException {
   MagicSquaresCormac magicSquaresCormac =  new MagicSquaresCormac();
    String[] files = {"Mercury.txt", "Luna.txt"};
    for (String file : files) {
        System.out.println(file + " (horizontal lines) is magic? " + magicSquaresCormac.horizontalLine(file));
        System.out.println(file + " (vertical lines) is magic? " + magicSquaresCormac.verticalLine(file));
        System.out.println(file + " (diagonal line) is magic? " + magicSquaresCormac.horizontalLine(file));
    }
}
}

Answer 1

I suspect that you should read the matrices from files only once and store them in a two-dimensional integer array. Once you have that, it is trivial to verify whether the squares are magic.

I had in mind something like this:

private static int[][] loadFile(File file) {
     // Do file I/O for loading the square
     return square;
}

private static int getRowMagic(int[][] square, int row) {
    int sum = 0;

    for (int column = 0; column < square.length; ++column) {
        sum += square[row][column];
    }

    return sum;
}

private static int getColumnMagic(int[][] square, int column) {
    int sum = 0;

    for (int row = 0; row < square.length; ++row) {
        sum += square[row][column];
    }

    return sum;
}

private static int getDescendingDiagonalMagic(int[][] square) {
    int sum = 0; 

    for (int i = 0; i < square.length; ++i) {
        sum += square[i][i];
    }

    return sum;
}

private static getAscendingDiagonalIsMagic(int[][] square) {
    int sum = 0;

    for (int i = 0; i < square.length; ++i) {
        sum += square[square.length - 1 - i][i];
    }

    return sum;
}

public static boolean isMagicSquare(int[][] square) {
    int sum = getRowMagic(square, 0);

    for (int i = 0; i < square.length; ++i) {
        if (sum != getRowMagic(square, i) || sum != getColumnMagic(square, i)) {
            return false;
        }
    }

    return getAscendingDiagonalMagic(square) == sum &&
           getDescendingDiagonalMagic(square) == sum;
}

Hope that helps.

Answer 2

Pointless constructor

The return statement is unnecessary as the last statement in a constructor. Actually you don't need the MagicSquaresCormac constructor at all, you can safely remove it.

Unnecessary assignments

These assignments are unnecessary:

String line = null;
Integer num = null;

Declare things as late as possible

Roughly speaking, the live time of a variable is the number of lines between declaration and last use. To minimize accidental misuses (window of vulnerability), it's best to declare variables as close to their uses as possible.

line should be declared just above the while loop. And without assigning a value to it, as it will always receive a value anyway.

num should be declared inside the for loop later where it's used, and it doesn't need to be an Integer, a simple int will do:

int num = Integer.valueOf(character);

The same goes for charList, you can declare it where you assign it:

List<String> charList = Arrays.asList(lineFromList.split("\\s"));

And so for numlist too:

List<Integer> numlist = new ArrayList<>();

Other unnecessary elements

You create listOfNums and put values in it, but then it's never used.

The line.split("\t"); does absolutely nothing.

Use try-with-resources

Instead of manually closing resources, it's better to use try-with-resources.

Putting it together

Applying the above suggestions, getNumberList becomes:

private List<List<Integer>> getNumberList(String file) throws IOException {
    List<String> lineList = new ArrayList<>();

    try (
            FileReader fr = new FileReader(file);
            BufferedReader br = new BufferedReader(fr);
    ) {
        String line;
        while ((line = br.readLine()) != null) {
            if (!line.equals("")) {
                lineList.add(line);
            }
        }
    }

    List<List<Integer>> numListList = new ArrayList<>();

    for (String lineFromList : lineList) {
        List<Integer> numlist = new ArrayList<>();
        List<String> charList = Arrays.asList(lineFromList.split("\\s"));
        for (String character : charList) {
            int num = Integer.valueOf(character);
            numlist.add(num);
        }
        numListList.add(numlist);
    }

    return numListList;
}

Going further...

You can apply similar refactoring techniques in your other methods. I leave that as an exercise for you. Just a few hints for horizontalLine:

• Replace Integer with int when possible
• You can completely eliminate isMagic by returning directly
• sum can be declared inside the for loop

Answer 3

You might also try using a try-with-resources statement to ensure your BufferedReader is always closed, even in the event of an Exception.