1
\$\begingroup\$

I wrote a python program to find if a matrix is a magic square or not. It works, but I can't help feeling like I may have overcomplicated the solution. I have seen other implementations that were a lot shorter, but I was wondering how efficient/inefficient my code is and how I could improve it and/or shorten it to achieve the result I am looking for.

def main():

    matrix = [[4,9,2],
              [3,5,7],
              [8,1,6]]
    
    result = loShu(matrix)

    print(result)


def loShu(matrix):

    i = 0
    j = 0

    for i in range(0, len(matrix)):
        for j in range(0, len(matrix[j])):
            if ((matrix[i][j] < 1) or (matrix[i][j] > 9)):
                return ("This is not a Lo Shu Magic Square - one of the numbers is invalid")

    row1 = matrix[0][0] + matrix[0][1] + matrix[0][2]
    row2 = matrix[1][0] + matrix[1][1] + matrix[1][2]
    row3 = matrix[2][0] + matrix[2][1] + matrix[2][2]

    ver1 = matrix[0][0] + matrix[1][0] + matrix[2][0]
    ver2 = matrix[0][1] + matrix[1][1] + matrix[2][1]
    ver3 = matrix[0][2] + matrix[1][2] + matrix[2][2]

    diag1 = matrix[0][0] + matrix[1][1] + matrix[2][2]
    diag2 = matrix[0][2] + matrix[1][1] + matrix[2][0]

    checkList = [row1,row2,row3,ver1,ver2,ver3,diag1,diag2]

    temp = checkList[0]

    for x in range (0, len(checkList)):
        if checkList[x] != temp:
            return ("This is not a Lo Shu Magic Square")

    return ("This is a Lo Shu Magic Square")


main()
\$\endgroup\$
1
  • \$\begingroup\$ Seems wrong, as it claims that [[1,1,1], [1,1,1], [1,1,1]] is a Lo Shu Magic Square. \$\endgroup\$
    – Manuel
    Commented Apr 17, 2021 at 17:45

1 Answer 1

4
\$\begingroup\$

Your solution is hardcoded for matrices of size 3x3 - why not support matrices of arbitrary size as well? It's also difficult to maintain and debug your code, i.e., it's easy to mess up somewhere when you are indexing your rows and columns. Also, imagine you wanted to consider larger matrices: what a nightmare to now modify your existing logic!

A more robust solution could turn to numpy. Specifically, you can check row and column sums by doing m.sum() and m.sum(axis=1), and compute the sums of the diagonals via m.trace() or by sum(np.diag(m)) and sum(np.diag(np.fliplr(m)).

As a general comment, just return a boolean value from your function. Let the main program decide what do with this value, i.e., whether to print "this is good", "This is a Lo Shu Magic Square", or whatever. In this way, your verifier function stays clean and more generic.

\$\endgroup\$
1
  • \$\begingroup\$ That makes sense. This is exactly what I was looking for - a way other than hardcoding. Thank you! \$\endgroup\$
    – am2021
    Commented Apr 17, 2021 at 17:42

Not the answer you're looking for? Browse other questions tagged or ask your own question.