2
\$\begingroup\$

I wanted to know if someone can find a better way to code this in C++ using 1 or 2 dimensional arrays instead of int tables. It has to be a static 7x7 array where the user can make a 3x3 or 5x5 or 7x7 magic square.

Static allocation and is allocated on a stack and not a heap, and static array or static matrices.

Write a program that queries the user for an odd integer n, where n is a 3, a 5, or a 7. Create a 7x7 static matrix and use it to produce an n x n magic square as described in the problem..

Static arrays or static matrices must be used for this assignment; that is memory for the matrix must be allocated at compile time; solutions that do not use static allocation are unacceptable.

Here is my code

/*--------------------------------------------------------------------
 Program to construct magic squares of odd size.
 Input: Size of square (odd #)
 Output: A magic square
 ----------------------------------------------------------------------*/
#include <iostream>      // cin, cout, >>, <<
#include <iomanip>      // setw()
#include <cstdlib>      // exit()
using namespace std;

const int MAX_DIM = 7;
typedef int IntTable[MAX_DIM][MAX_DIM ];

void createMagic(IntTable square, int n);
/*---------------------------------------------------------------------
 Construct a magic square of odd size n.
 Precondition: size of square is odd positive integer.
 Postcondition: square stores an n x n magic square.
 ----------------------------------------------------------------------*/

void display(IntTable t, int n);
/*---------------------------------------------------------------------
 Display an n x n magic square.
 Precondition: None.
 Postcondition: square is output to cout.
 ----------------------------------------------------------------------*/

int main()
{
    IntTable square;
    int sizeOfSquare;
    cout << "\nEnter size of magic square (odd number): ";
    cin >> sizeOfSquare;
    createMagic(square, sizeOfSquare);
    display(square, sizeOfSquare);
}

//--- Definition of createMagic()
void createMagic(IntTable square, int n)
{
    if((n % 2 == 0) || n < 3 || n > 7) { cerr << "Size of magic square must be odd and between 3 and 7 (inclusive).\n";
     exit(1); 
     }

    int row,
        col;
    for (row = 0; row < n; row++)
        for (col = 0; col < n; col++)
            square[row][col] = 0;
    row = 0;
    col = n / 2;
    for (int k = 1; k <= n * n; k++)
    {
        square[row][col] = k;
        row--;
        col++;
        if (row < 0 && col >= n)
        {
            row += 2;
            col--;
        }
        if (row < 0)
            row = n - 1;
        if (col >= n)
            col = 0;
        if (square[row][col] != 0)
        {
            row += 2;
            col--;
        }
    }
}

//--- Definition of display()
void display(IntTable t, int n)
{
    const int WIDTH = 4;
    cout << "\nMagic square is:\n" << endl;
    for (int i = 0; i < n; i++)
    {
        for(int j = 0; j < n; j++)
            cout << setw(WIDTH) << t[i][j];
        cout << endl << endl;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ didn't get what magic square means, can u explain that? \$\endgroup\$ Sep 22 at 5:13
0
\$\begingroup\$

a bad habit

using namespace std;

Don't get into this habit. It can lead to name collisions. It's best to specify the full namespace where necessary, e.g. std::cout << ...


prefer std::array to c-style arrays, use a class for convenience

const int MAX_DIM = 7;
typedef int IntTable[MAX_DIM][MAX_DIM];

Obviously a std::vector of the correct size, allocated at run-time, would be ideal, but we could use a std::array as the next best thing.

MAX_SIZE might be a better name than MAX_DIM. Your table always has two dimensions. We really want the "max size of a dimension" for this constant.

Consider using a class instead, so that we don't have to pass the size and table around separately, e.g.:

template<int MaxSize>
class MagicSquare
{
    int m_size;
    std::array<std::array<int, MaxSize>, MaxSize> m_data;

public:
    
    explicit MagicSquare(int size) { ... }
    void display() { ... }
};

comments and maintenance burden

void createMagic(IntTable square, int n);
/*---------------------------------------------------------------------
Construct a magic square of odd size n.  Precondition: size of square
is odd positive integer.  Postcondition: square stores an n x n magic
square.
----------------------------------------------------------------------*/

We could put the full function definition here, instead of having a separate declaration. As well as being less typing, this would mean that when we change the function signature, we only have to change it in one place, not two.

It's worth mentioning and / or describing the algorithm used to construct the magic square in the comment for this function.

Comments are usually placed above the code they describe.


use named constants, not "magic numbers"

if((n % 2 == 0) || n < 3 || n > 7) { cerr << "Size of magic square must be odd and between 3 and 7 (inclusive).\n";
 exit(1); 
 }

We should probably use our MAX_DIM constant here, and could have a MIN_DIM too.

Good job using std::cerr for error output.

It would be better to use the named constant from the <cstdlib> header: exit(EXIT_FAILURE); instead of a numeric 1.

The createMagic function doesn't actually need input to be quite as restrictive. Fetching the user input and ensuring that it's in a valid range should be done outside of this function. createMagic could then use simple assertions to ensure it has valid input.


declaring and using variables

int row,
    col;
for (row = 0; row < n; row++)
    for (col = 0; col < n; col++)
        square[row][col] = 0;
row = 0;
col = n / 2;

Don't declare multiple variables on the same line (it can get complicated).

Declare variables in the smallest scope possible, and don't reuse them (it's easy to forget to reset a value).

So we should do:

for (int row = 0; row < n; ++row)
    for (int col = 0; col < n; ++col)
        square[row][col] = 0;

int row = 0;
int col = n / 2;

pre- vs post- increment

    row--;
    col++;

Although they optimize to the same thing here, note that pre-increment and post-increment have different meanings.

We should use pre-increment operator (e.g. ++row) unless we actually need the temporary value created by the post-increment operator.


algorithm simplifications

row = 0;
col = n / 2;
for (int k = 1; k <= n * n; k++)
{
    square[row][col] = k;
    row--;
    col++;
    if (row < 0 && col >= n)
    {
        row += 2;
        col--;
    }
    if (row < 0)
        row = n - 1;
    if (col >= n)
        col = 0;
    if (square[row][col] != 0)
    {
        row += 2;
        col--;
    }
}

I think we can simplify this somewhat:

  • Although descriptions of this algorithm talk about "if we hit a filled square", note that this always happens after exactly n moves (since we're travelling along a diagonal). So we don't actually have to check this condition explicitly, we just make sure we make the correct number of "up and right" moves.

  • It's neater to move all the shenanigans needed for incrementing or decrementing and then wrapping an index to a separate function.

Perhaps something like:

auto const inc = [&] (int x) { return x == n - 1 ? 0 : x + 1; };
auto const dec = [&] (int x) { return x == 0 ? n - 1 : x - 1; };

auto x = n / 2;
auto y = 0;
auto count = 1;

for (auto j = 0; j != n; ++j)
{
    for (auto i = 0; i != n - 1; ++i)
    {
        square[y][x] = count++;

        x = inc(x);
        y = dec(y);
    }

    square[y][x] = count++;
    
    y = inc(y);
}

Note the use of post-increment only where it's actually needed.

Note that the last value in a diagonal is set separately so we don't have to "undo" an unnecessary "up and right" move.


checking and testing

It might be worth implementing a function to check that the resulting table is a valid magic square. Then you could write some unit tests with a unit testing library (e.g. Catch2 or GoogleTest) to check correctness with different inputs.

\$\endgroup\$
1
  • \$\begingroup\$ Using un-delimited code blocks if (foo) bar = foo; is a very bad habit, it can too easily result in compile-able flow errors when modifying code. Down voting due to this. \$\endgroup\$
    – Blindman67
    Sep 23 at 2:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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