0
\$\begingroup\$

I've written a simple program using modular programming in C (using a lot of functions), that gets a line of input from the user, and then prints if a given input forms a magic square or not.

The first input is the order of the matrix, and the rest are order*order inputs(order^2)(integers) that are actually filling the matrix. So if I put for instance 3 1 2 3 4 5 6 7 8 9 the order will be 3 and the inputs will be 1,..,9.

One thing that I am not sure is how to handle inputs less than 9.

Anyway, I would like to see your tweaks and learn more and improve from your blessed share of knowledge.

Please keep in mind it should be written in ANSI-C (I know it's old and all, but those are the requirements). If you have something cool to share regarding C11 or others, please do so as I like to extend my knowledge and improve (even though I'm a noob now).

#include <stdio.h>
#include <stdlib.h>
#define MAX 10 /*maximal size of the array*/



void checkOrderRange(int order, int matrix[order][order]); /* checking if number of rows+number of columns is greater than N^2 */
void checkOrderType(int order);                            /*checking if order is a digit*/
void checkOrderSize(int order);                            /*checking if order is in the correct size (bigger than 3 and smaller than selected MAX) */
void scanMatrix(int order, int matrix[order][order]);      /*scans the matrix and assigns given values */
void printMatrix(int order, int matrix[order][order]);     /*prints given matrix*/
int diagonalSumI(int order, int matrix[order][order]);     /*calculates first diagonal sum */
int diagonalSumII(int order, int matrix[order][order]);    /*calculates second diagonal sum */
int columnSumAndCompareToDiagonal(int order, int matrix[order][order]); /*calculates sum of each coloumn and stores in an array*/
int rowSumAndCompareToDiagonal(int order, int matrix[order][order]);    /*calculates sum of each row and stores in an array*/
void checkSum(int order, int matrix[order][order]);                     /*compares above sums to check if it's indeed a magic square*/
int isdigit();
void checkIfMagicSquare(int order, int matrix[order][order]);

int main() {
    int order, matrix[MAX][MAX];
    printf("please enter the order of the magic square \n");
    scanf("%d", &order);
    scanMatrix(order, matrix);
    checkOrderSize(order);
    printf("here's your matrix \n");
    printMatrix(order, matrix);
    columnSumAndCompareToDiagonal(order, matrix);
    rowSumAndCompareToDiagonal(order, matrix);
    checkIfMagicSquare(order,matrix);

    return 0;
}

void scanMatrix(int order, int matrix[order][order]) {
    for (int I = 0; I < order; I++) {
        for (int J = 0; J < order; J++) {
            if (scanf("%d", &matrix[I][J]) != 1) {
                puts("you did not enter an integer(decimal)\n"); /* checking for integer insertion*/
                exit(0);
            }
        }
    }
}

void printMatrix(int order, int matrix[order][order]) {
    for (int I = 0; I < order; I++) {
        for (int J = 0; J < order; J++) {
            printf("%d\t", matrix[I][J]);
        }
        printf("\n");
    }
}

int diagonalSumI(int order, int matrix[order][order]) {
    int diagonal = 0;
    for (int I = 0; I < order; I++) {
        diagonal = diagonal + matrix[I][I];
    }
    return diagonal;
}

int diagonalSumII(int order, int matrix[order][order]) {
    int diagonal2 = 0;
    for (int I = 0, J = order - 1; I < order; I++, J--) {
        diagonal2 = diagonal2 + matrix[I][J];
    }
    return diagonal2;
}

int columnSumAndCompareToDiagonal(int order, int matrix[order][order]) {
    int columnsum[order];
    for (int I = 0; I < order; I++) {
        columnsum[I] = 0;
        for (int J = 0; J < order; J++) {
            columnsum[I] = columnsum[I] + matrix[I][J];
        }
    }
    for (int K = 0; K < order; K++) {
        if (columnsum[K] == diagonalSumI(order, matrix) && diagonalSumI(order, matrix) == diagonalSumII(order, matrix)) {}
        else {
            fprintf(stderr, "Sorry. but not a magic square\n");
            exit(EXIT_FAILURE);
            return 0;
        }

    }
    return 1;
}

int rowSumAndCompareToDiagonal(int order, int matrix[order][order]) {
    int rowsum[order];
    for (int J = 0; J < order; J++) {
        rowsum[J] = 0;
        for (int I = 0; I < order; I++) {
            rowsum[J] = rowsum[J] + matrix[I][J];
        }}
        for (int K = 0; K < order; K++) {
            if (rowsum[K] == diagonalSumI(order, matrix) && diagonalSumI(order, matrix) == diagonalSumII(order, matrix)) {
            } else {
                fprintf(stderr, "Sorry. but not a magic square\n");
                exit(EXIT_FAILURE);
                return 0;
            }

        }
    return 1;
}
void checkIfMagicSquare(int order, int matrix[order][order]) {
    if (rowSumAndCompareToDiagonal(order, matrix) && columnSumAndCompareToDiagonal(order, matrix)) {
        printf("congratulations, it is a magic square \n");
    } else {
        fprintf(stderr, "Sorry. but not a magic square \n");
    }
}

void checkOrderSize(int order) { /*checking if order is in the correct size (bigger than 3 and smaller than selected MAX) */
    if (order < 3 || order > MAX) {
        fprintf(stderr, "order of matrix is either smaller than 3 or larger than %d\n", MAX);
        exit(EXIT_FAILURE);
    }
}

void checkOrderType(int order) { /*checking if order is a digit*/
    if (!isdigit(order)) {
        fprintf(stderr, "Input is not decimal or not a digit\n");
        exit(EXIT_FAILURE);
    }
}
\$\endgroup\$
  • \$\begingroup\$ some of it in c11, but i need to revert it to ansi. this is why i wrote ansi \$\endgroup\$ – BeginningMath Dec 5 '17 at 12:07
  • \$\begingroup\$ plus if you see checks that i missed, please note and if you can show me how to fix them. wanna do my best \$\endgroup\$ – BeginningMath Dec 5 '17 at 12:08
  • \$\begingroup\$ Your for (int I = …; declarations are currently not ANSI C compliant. \$\endgroup\$ – 200_success Dec 5 '17 at 14:23
  • \$\begingroup\$ Format to the review's standard. If the horizontal scroll bar appears, code is wider than it should be for review. Recommend an auto formatter. \$\endgroup\$ – chux Dec 14 '17 at 3:26
  • \$\begingroup\$ I do not understand what you mean by "One thing that I am not sure is how to handle inputs less than 9." Your example has several inputs that are less than 9, and it appears to handle them fine. \$\endgroup\$ – Snowbody Dec 14 '17 at 5:49
5
\$\begingroup\$

Missing Error Checks

Always check user input, you are doing this correctly in the function scanMatrix() but you missed the check on order before scanMatrix() is called.

Prefer return EXIT_status; Over exit(EXIT_status);

There are a number of functions that call exit(EXIT_FAILURE); In a small program where only one person is doing the program it's very difficult to see the problem with this, however, many programs written in C such as operating systems should never exit, they need to handle errors in another fashion. Often a program will have to delete allocated memory, close files after an error occurs or send a signal to another program before it exits.

It's always best to exit a program from main() using return.

You've got a good start here, what should be done is that any void function that calls exit(EXIT_FAILURE); should instead be a function that returns integer and the exit() should be modified to return EXIT_FAILURE; Upon success use return EXIT_SUCCESS;.

int main(int argc, const char * argv[]) {
    int order;
    int matrix[MAX][MAX];
    int exit_status = EXIT_SUCCESS;

    printf("please enter the order of the magic square \n");
    scanf("%d", &order);
    exit_status = scanMatrix(order, matrix);

    if (exit_status == EXIT_SUCCESS)
    {
        exit_status = checkOrderSize(order);
        if (exit_status == EXIT_SUCCESS)
        {
            printf("here's your matrix \n");
            printMatrix(order, matrix);

            columnSumAndCompareToDiagonal(order, matrix);
            rowSumAndCompareToDiagonal(order, matrix);
            checkIfMagicSquare(order,matrix);
        }

    }

    return exit_status;
}

*Note: Some compilers will flag the following because the return statement can't be reached:

            } else {
                fprintf(stderr, "Sorry. but not a magic square\n");
                exit(EXIT_FAILURE);
                return 0;
            }

Simplify the Error Checking

The code contains a number of conditionals (if (CONDITION) {THEN block} else {ELSE block}) where the CONDITION is a check for the absence of an error, that have an empty THEN block, and handle the error in the ELSE block. Why not make the condition test for the presence of the error, then handle the error in the THEN block and remove the ELSE block?

An example:

    for (int K = 0; K < order; K++) {
        if (columnsum[K] == diagonalSumI(order, matrix) &&
            diagonalSumI(order, matrix) == diagonalSumII(order, matrix))
        {}
        else {
            fprintf(stderr, "Sorry. but not a magic square\n");
            exit(EXIT_FAILURE);
            return 0;
        }

    }

Could be changed to :

    for (int K = 0; K < order; K++)
    {
        if (columnsum[K] != diagonalSumI(order, matrix) ||
                diagonalSumI(order, matrix) != diagonalSumII(order, matrix))
        {
            fprintf(stderr, "Sorry. but not a magic square\n");
            return(EXIT_FAILURE);
        }
    }

Keep in mind that less code provides less chance for errors.

Comment Style

The ANSI standard, C99, and C11 all allow end of line comments starting with //. Block comments (/* ... */) have their purposes but it's safer to use // for most comments because they end at the end of the line. A missing */ can be hard to find when IDEs with automatic syntax highlighting are not available.

\$\endgroup\$
  • \$\begingroup\$ thank you very much for your comment @pacmaninbw, how can i make it ansi-compatible? \$\endgroup\$ – BeginningMath Dec 5 '17 at 14:53
  • \$\begingroup\$ Some compilers allow you to choose the standard. The compile line option would be -std="RequestedStandard". You want to compile using the C89 standard. \$\endgroup\$ – pacmaninbw Dec 5 '17 at 14:57
  • 1
    \$\begingroup\$ if (!scanf(....)) { report error; return EXIT_FAILURE; } else { exit_status = function() } \$\endgroup\$ – pacmaninbw Dec 5 '17 at 15:36
  • 1
    \$\begingroup\$ see techonthenet.com/c_language/standard_library_functions/stdio_h/… \$\endgroup\$ – pacmaninbw Dec 5 '17 at 15:42
  • 1
    \$\begingroup\$ Format thought: for (int K = 0; K < order; K++) { with it brace at the end-of-line and if (columnsum[K] != diagonalSumI(order, matrix) || diagonalSumI(order, matrix) != diagonalSumII(order, matrix)) { with it brace on the next line looks inconsistent. I'd recommend the first style, or the 2nd, but not mixed. \$\endgroup\$ – chux Dec 14 '17 at 3:30
3
\$\begingroup\$

Here are the problems I see:

  • The code isn't modular enough

    Each function should have one job -- this is called the single responsibility principle. This makes it easy both to test that the code is correct, and to reuse the code elsewhere. Right now you have functions doing several unrelated things, that would be better off encapsulated.

  • Dead code

    The function checkOrderType(), with a prototype and an implementation, is never called, and for good reason. The standard library function isdigit() from <ctype.h>, although it is declared to take an int, actually expects an unsigned char, that is, a representation of a character. (As @Roland Illig said, the only reason it's declared as int is so it can also handle EOF.) It is not applicable/meaningful for numeric values. It is the wrong function for your purposes. Just check normally with < MAX.

  • #define MAX 10 /*maximal size of the array*/
    

    Why not use a more descriptive identifier name? Also the comment is ambiguous, and I think you even confused yourself as a result: The code uses MAX inconsistently. The sanity check functions only allow progress if the order is strictly less than than MAX, but the array is declared large enough to support matrices with an order up to and including MAX. What did you intend here?

  • void checkOrderRange(int order, int matrix[order][order]);
    /* checking if number of rows+number of columns is greater than N^2 */
    

    This function does not appear to be implemented. Why is this prototype here? In any event this prototype does not meet ANSI C standards -- all dimensions of the array except the outermost (leftmost) must be constants. I think this is required to be int matrix[][MAX]

  • void checkOrderSize(int order);
    /*checking if order is in the correct size (bigger than 3 and smaller than selected MAX) */
    

    Why do you say "selected MAX" here? Who is selecting it? It seems to me that it's defined as a constant by the developer and can never be changed.

  • void scanMatrix(int order, int matrix[order][order]);
    /*scans the matrix and assigns given values */
    

    This comment is utterly incomprehensible. How about "reads a square matrix with specified order from standard input and places the values in the second parameter"? Also fix the type of the parameter to be ANSI compliant.

  • void printMatrix(int order, int matrix[order][order]);     /*prints given matrix*/
    

    "Print" is ambiguous. How about "sends a text representation of the matrix to standard output"?

  • int diagonalSumI(int order, int matrix[order][order]);     /*calculates first diagonal sum */
    int diagonalSumII(int order, int matrix[order][order]);    /*calculates second diagonal sum */
    

    How about calling them "principal diagonal" and "counter diagonal", like mathematicians do? Anyway, all of these have the same ANSI noncompliance as stated above, and need to be fixed in the same way.

  • int columnSumAndCompareToDiagonal(int order, int matrix[order][order]); /*calculates sum of each coloumn and stores in an array*/
    int rowSumAndCompareToDiagonal(int order, int matrix[order][order]);    /*calculates sum of each row and stores in an array*/
    
    1. spell check "column"
    2. What array? Why is an array even being mentioned here? Each of these functions returns an int, not an array. The comments by the prototype should indicate what the function does (its purpose in life), not how it does it (its implementation).
    3. Why isn't the diagonal sum passed in as a parameter so it doesn't have to be recomputed?
  • void checkSum(int order, int matrix[order][order]);
    /*compares above sums to check if it's indeed a magic square*/
    

    Another unimplemented and uncalled function.

  • int isdigit();
    

    If you're going to call a standard library function, you should #include the proper header file, not declare the prototype yourself. You didn't even get the prototype right (I wouldn't have gotten it right either).

  • void checkIfMagicSquare(int order, int matrix[order][order]);
    

    By C tradition, "check" functions should return 1 for success and 0 for failure.

  • int main() {
       int order, matrix[MAX][MAX];
       printf("please enter the order of the magic square \n");
       scanf("%d", &order);
    

    You need to validate the user input and handle the case if the user types something that is not a number. These two lines should probably be a call to separate function e.g. order = getIntFromKeyboard("please enter the order of the magic square\n");

  • scanMatrix(order, matrix);
    checkOrderSize(order);
    

    Shouldn't these two lines be switched?

  • printf("here's your matrix \n");
    printMatrix(order, matrix);
    columnSumAndCompareToDiagonal(order, matrix);
    rowSumAndCompareToDiagonal(order, matrix);
    checkIfMagicSquare(order,matrix);
    

    You are calling columnSumAndCompareToDiagonal() twice, both here and in checkIfMagicSquare(), which is called here. Likewise for rowSumAndCompareToDiagonal(). The ones here are useless since the return values are being thrown away.

  • void scanMatrix(int order, int matrix[order][order]) {
        for (int I = 0; I < order; I++) {
            for (int J = 0; J < order; J++) {
                if (scanf("%d", &matrix[I][J]) != 1) {
                    puts("you did not enter an integer(decimal)\n"); /* checking for integer insertion*/
                    exit(0);
    

    It would be more user-friendly to give the user a chance to re-enter their input properly. Maybe a do {} while (); loop.

  • int columnSumAndCompareToDiagonal(int order, int matrix[order][order]) {
        int columnsum[order];
    

    Not ANSI compliant. The array size must be a constant. It's okay to use MAX.

  • for (int I = 0; I < order; I++) {
        columnsum[I] = 0;
        for (int J = 0; J < order; J++) {
            columnsum[I] = columnsum[I] + matrix[I][J];
        }
    }
    for (int K = 0; K < order; K++) {
        if (columnsum[K] == diagonalSumI(order, matrix) && diagonalSumI(order, matrix) == diagonalSumII(order, matrix)) {}
    

    This is inefficient. You are recomputing diagonalSumI() twice and diagonalSumII() once for each column in the matrix, even though their values don't change. Call each of them once, store the value in a variable, and then pass them into the row and column functions.

    Also, what's the reason for storing the sums in an array anyway? You never work with more than one at a time.

  • void checkIfMagicSquare(int order, int matrix[order][order]) {
        if (rowSumAndCompareToDiagonal(order, matrix) && columnSumAndCompareToDiagonal(order, matrix)) {
            printf("congratulations, it is a magic square \n");
        } else {
            fprintf(stderr, "Sorry. but not a magic square \n");
    

    What's the reason you sometimes printf() (to stdout) and other times fprintf(stderr,...)? What is the output of the program defined as? stderr is defined as not being program output; it is supposed to be used for diagnostic and debugging purposes.

  • void checkOrderType(int order) { /*checking if order is a digit*/
        if (!isdigit(order)) {
            fprintf(stderr, "Input is not decimal or not a digit\n");
            exit(EXIT_FAILURE);
        }
    }
    

As said before, this function does not work, and should probably be removed.

\$\endgroup\$
  • 1
    \$\begingroup\$ isdigit doesn't expect a char as argument, it can only handle unsigned char and EOF. \$\endgroup\$ – Roland Illig Dec 18 '17 at 21:32
1
\$\begingroup\$

I think your code could be more compact by making foreach-functions for rows, elements and columns and use them to implement some of the other functions. pseudo:

void foreachelement(int* matrix, int order, void* data, void (f*)(int * element, void* data)) {
  for(row...) {
    for(col...) {
      f(matrix_element..., data);
    }
  }
}

void sum(int* element, void* data) {
  *(int*)data += *element;
}

int matrix[3][3]; // todo: set some values
int result = 0;
foreachelement(matrix, 3, &result, sum);

Your error handling looks a lot like assertions. Add #include <assert.h> and change your checks (when you would otherwise print to stderr and exit(-1); to something similar to:

assert(isdigit(order));

If in need you can easily insert a more elaborate message in assert() using the sequencing operator and a string constant:

assert(("Input is not decimal or not a digit", isdigit(order));

assert is a timesaver for us developers when focusing on correct (user) behavior first and error handling later. besides, one of these days you may need to integrate your code in an application with stderr piped to null or not otherwise displayed.

\$\endgroup\$

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.