HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Magic square functions

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
functionsmagicsquare

Problem

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 
#include 
#include 

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 = z ) { e = 0; } if ( magic[d][e] != 0 )
            {
                d = d + 2;
                e = e - 1;
            }
            magic[d][e] = r;
        }
        for(r=0;r > c;
    while(c >= 20)
    {
        cin.clear();
        cin.ignore(numeric_limits::max(),'\n');
        cout > c;
    }
    return c;
}
int oddfunction(int &a) 
{
    int cont=0;
    while(cont == 0)
    {
        cout > h;
    if(h == i)
    {
        exit = 0;
        return exit;
    }
    else
    {
        exit = 1;
        return exit;
    }
}

Solution

-
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 = 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.

Code Snippets

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

Context

StackExchange Code Review Q#72291, answer score: 3

Revisions (0)

No revisions yet.