Magic square functions

Question

I'm writing a program for magic square and I'm a relative beginner to C++ and programming. I was hoping someone could help me find alternate or better ways to implement my code. This is the main function. I also entered a function to check if the input is odd, max 19, and to quit.

Is there a way to not have back-to-back for and if statements?

#include <iostream>
#include <limits>
#include <iomanip>

using namespace std;
int oddfunction(int &a);
int endfunction(int &exit);
int maxfunction();  
int main()
{ 
    int r, c, d, e, z=0;
    int *f = new(nothrow) int;
    int *g= new(nothrow) int;
    int magic[120][120];
    cout << "EML 2032 Final Project: Magic Square (odd numbers) " << endl;

    while(*g != 0)
    {
        cout << "\n\nMagic Squares for integers less than 20. ";
        for ( r=0 ; r<120 ; r++ )
        for ( c=0 ; c<120 ; c++ )
        magic[r][c] = 0;
        *f = oddfunction(*f);
        z = *f;
        cout <<"\n\n";
        d = 0 ; 
        e = (z-1) / 2;
        magic[d][e] = 1;
        for ( r=2 ; r <= z * z ; r++ )
        {
            d--; 
            e++;
            if ( d < 0 && e == z )
            {
                d  = d + 2;
                e--;
            }
            if ( d < 0 ) { d = z - 1; } if ( e >= z ) { e = 0; } if ( magic[d][e] != 0 )
            {
                d = d + 2;
                e = e - 1;
            }
            magic[d][e] = r;
        }
        for(r=0;r < z;r++)
        {
            for(c = 0; c < z; c++)
            {
                cout << setw(4) << magic[r][c];
            }
            cout << endl;
        }
        endfunction(*g);   
   }
   cout << endl;
   cout << "Have a nice day!        THE END \n";
   system("pause");
   return 0;
}

int maxfunction() 
{
    int c=0;
    cin >> c;
    while(c >= 20)
    {
        cin.clear();
        cin.ignore(numeric_limits<int>::max(),'\n');
        cout << "Input an odd integer up to 19.\n";
        cin >> c;
    }
    return c;
}
int oddfunction(int &a) 
{
    int cont=0;
    while(cont == 0)
    {
        cout << "Input an odd number: " << endl;
        a = maxfunction();
        if(a % 2 == 0 )
        {
            if(a % 2 == 0 )
            {
                cout << "That wasn't odd! (odd numbers aren't divisible by 2)\nEnter any number to continue:" << endl;
                a = maxfunction();
            }
            else
            {
                a = maxfunction();
                cont++;
            }
        }
        else
        {
            cont++;
        }
    }
    return a;
}
int endfunction(int &exit)
{
    int h=0;
    int i=2032;
    h = exit;
    cout << "\n-Input 2032 to quit... \n-Input any other number to continue..." << endl;
    cin >> h;
    if(h == i)
    {
        exit = 0;
        return exit;
    }
    else
    {
        exit = 1;
        return exit;
    }
}

Answer 1

• Naming

  Avoid single-letter identifiers. r, c, d, e, z - who are those guys? What's their purpose? I need to thoroughly study the code to understand that, and then I'd immediately forgot them. Make my life easier.

  oddfunction is really get_odd_number

• Spacing

  if ( d < 0 ) { d = z - 1; } if ( e >= z ) { e = 0; } if ( magic[d][e] != 0 )
  

  is not possible to read. Sparing lines will not make your program run any better, but makes understanding and maintaining it much harder. I hope it is a copy-paste problem.

• Redundancy

  f and g do not have a right to exist. Especially they do not have right to exist as pointers. Also, you should make up your mind and return the result e.g. of oddfunction either by reference or by return value.

• Separation of responsibilities

  oddfunction prompts for an odd number, and calls maxfunction, which also prompts for an odd number, this time stating the limit. Double maintenance problem right away. Make one function to deal with IO (prompt and read), and another with business logic (validation). The same validation function shall recognize 2032 as a termination condition.

  Along the same line, separate output from the calculations.

• The algorithm is not reviewed, mostly due to the naming problem. An introductory comment (or a link to the algorithm) would also be helpful.

Answer 2

Many things look quite wrong from a logic/organisation point of view.

In endfunction:

• You have return exit; in the two branches which doesn't make much sense. Just move it after the end of the else.

• You give h the value 0, then the value of exit which is then overriden with cin.

• You do not use the value returned by endfunction. Instead of passing a parameter by reference, you could just use a return value.

• The name does not convey much information.

In oddfunction:

• You check if(a % 2 == 0 ) twice. Fore sure this doesn't make much sense.

• You could define cont as a boolean because that how you are going to use it.

• You could get rid of it alltogether by just returning when you get an odd value.

• The input/output is just really user friendly because you ask for the same information multiple times.

• The name does not convey much information.

In maxfunction:

• The name does not convey much information.

In main:

• Indentation and style are pretty weird here.

• This function is doing way too much. By defining a function to initialise a magic square and one to print it, you should be able to make things clearer.

• It is usually a good idea to define variables in the smallest possible scope.

• You have the value 120 hardcoded in multiple places.

• Things like d = d+2 can be written d+=2.

After fixing many details, your code looks like :

#include <iostream>
#include <limits>
#include <iomanip>

using namespace std;
int get_odd_number();
bool ask_to_continue();
int get_number();

static const int SIZE_MAX = 120; 
int main()
{ 
    int magic[SIZE_MAX][SIZE_MAX];
    cout << "EML 2032 Final Project: Magic Square (odd numbers) " << endl;

    do
    {
        cout << "\n\nMagic Squares for integers less than 20. ";
        for (int r=0 ; r<SIZE_MAX ; r++ )
            for (int c=0 ; c<SIZE_MAX ; c++ )
                magic[r][c] = 0;
        int z = get_odd_number();
        cout <<"\n\n";
        int d = 0 ; 
        int e = (z-1) / 2;
        magic[d][e] = 1;
        for (int r=2 ; r <= z * z ; r++ )
        {
            d--; 
            e++;
            if ( d < 0 && e == z )
            {
                d+=2;
                e--;
            }
            if ( d < 0 )
                d = z - 1;
            if ( e >= z )
                e = 0;
            if ( magic[d][e] != 0 )
            {
                d+=2;
                e--;
            }
            magic[d][e] = r;
        }
        for(int r=0 ; r < z; r++)
        {
            for(int c = 0; c < z; c++)
            {
                cout << setw(4) << magic[r][c];
            }
            cout << endl;
        }
    } while (ask_to_continue());
    cout << endl;
    cout << "Have a nice day!        THE END \n";
    system("pause");
    return 0;
}

int get_number() 
{
    int c=0;
    cin >> c;
    while(c >= 20)
    {
        cin.clear();
        cin.ignore(numeric_limits<int>::max(),'\n');
        cout << "Input an odd integer up to 19.\n";
        cin >> c;
    }
    return c;
}
int get_odd_number() 
{
    while (true)
    {
        int a = get_number();
        if(a % 2)
            return a;
        cout << "That wasn't odd! (odd numbers aren't divisible by 2)" << endl;
    }
}
bool ask_to_continue() // true to continue
{
    int h=0;
    cout << "\n-Input 2032 to quit... \n-Input any other number to continue..." << endl;
    cin >> h;
    return (h != 2032);
}