10
\$\begingroup\$

This is for a university assignment (don't worry the code is in working order), and while I could hand it in as-is and receive full marks I feel like there is probably a more efficient and better way to handle what I am doing.

What the code does

Takes user input of 4 names and for each name take True / False assignment scores in the format of 0 for False and 1 for True; then compare against the user input answer key and give: highest score, number correct for each name, and the letter grade which is based upon the highest score of the 4 names (more about that in the comments of the computeGrade() function)

Sample Input

Information for Student #1
Name: Smith
Answers: 0 1 0 0 1 1 1 0 0 1 1 1 0 0 0 1 1 0 0 1

Information for Student #2
Name: Tony
Answers: 0 1 0 0 1 1 1 0 0 0 0 0 1 1 1 0 0 1 1 1

Information for Student #3
Name: Patty
Answers: 1 1 0 0 1 1 1 0 0 0 0 0 1 1 1 0 0 1 1 1

Information for Student #4
Name: Meg
Answers: 1 0 1 1 0 1 1 0 0 1 1 1 0 0 0 1 1 0 0 0

Please enter the Answer Key: 0 1 0 0 1 1 1 0 0 1 1 1 0 0 0 1 1 0 0 0

Sample Output

Name      Answers                                      Number Correct      Grade
--------------------------------------------------------------------------------
Smith     0 1 0 0 1 1 1 0 0 1 1 1 0 0 0 1 1 0 0 1      19                  A
Tony      0 1 0 0 1 1 1 0 0 0 0 0 1 1 1 0 0 1 1 1      9                   F
Patty     1 1 0 0 1 1 1 0 0 0 0 0 1 1 1 0 0 1 1 1      8                   F
Meg       1 0 1 1 0 1 1 0 0 1 1 1 0 0 0 1 1 0 0 0      15                  B


Answer Key                                             Highest Score
--------------------------------------------------------------------
0 1 0 0 1 1 1 0 0 1 1 1 0 0 0 1 1 0 0 0                19

Concerned parts

Ultimately, I dislike the amount of for loops I'm using, especially in the void computeGrade() and void printAll() functions, that and I'm not sure if I'm just being OCD or if there's a better way to handle it. Also, I know my printf formatting probably isn't the best and I'm not as concerned with it as the rest of the program (but still slightly concerned). An overall review would be nice though as I come from being a beginner in Java to a beginner in C.

Side Notes:

  • I'm required to split the program up into the 4 functions you see in the code.

  • This may or may not annoy some of you, but the reason I've declared all my variables used in the for loops outside is because my university's compiler uses something before C99 and of course won't allow for (int i = 0; i < num; i++). (It was real bummer having it work in Visual Studio 2015 then have to fix it just to turn the assignment in.)


#define _CRT_SECURE_NO_WARNINGS // Required for Visual Studio 2015
#include <stdio.h>

#define NUM_STUDENTS 4 // Defining the number of students / For use in our arrays
#define NAMES_LENGTH 20 // Defining the maximum length of the names of the students / For use in our arrays
#define NUM_ANSWERS 20 // Defining the number of answers to be marked / For use in our arrays

void initialize(); // Initialize applicable arrays / Function prototype for Module 1000
void readData(); // Read in data / Function prototype for Module 2000
void computeGrade(); // Compute Grades / Function prototype for Module 3000
void printAll(); // Print all data / Function prototype for Module 4000

char sNames[NUM_STUDENTS][NAMES_LENGTH]; // Global Array for storing the names of the students
char sGrades[NUM_STUDENTS][1]; // Global Array for storing the grades of the students

int aKey[NUM_ANSWERS]; // Global Array for storing the answer key
int aCorrect[NUM_STUDENTS]; // Global Array for storing number of answers correct for the students
int sAnswers[NUM_STUDENTS][NUM_ANSWERS]; // Global Array for storing the answers of the students
int aHighest; // Global Variable to store the highest score

/* MODULE 0000 */

int main(void) {

    initialize(); // Call the initialize function to initialize aCorrect array to 0
    readData(); // Call the readData function to take user input
    computeGrade(); // Call the computeGrade function to calculate the grades of the students
    printAll(); // Call the printAll function to print all resulting data

    return 0;
}

/* MODULE 1000 */

void initialize() {

    int i;

    for (i = 0; i < 4; i++) {
        aCorrect[i] = 0;
    }

}

/* MODULE 2000 */

void readData() {

    int i, j;

    for (i = 0; i < 4; i++) {

        printf("Information for Student #%d\n", (i + 1)); // Label which student is being marked

        printf("Name: ", (i + 1)); // Prompt for the students name
        scanf("%s", sNames[i]); // Scan in the students name 

        printf("Answers: "); // Prompt for the answers

        for (j = 0; j < 20; j++) {
            scanf("%d", &sAnswers[i][j]); // Scan in the students answers
        }

        printf("\n");

    }

    printf("Please enter the Answer Key: ");

    int k;

    for (k = 0; k < 20; k++) {
        scanf("%d", &aKey[k]); // Scan in the answer key
    }

}

/* MODULE 3000 */

void computeGrade() {

    /* Compute the number of answers correct */

    int k, l;

    for (k = 0; k < 4; k++) {
        for (l = 0; l < 20; l++) {
            if (sAnswers[k][l] == aKey[l]) { // Compare students answer to the answer key
                aCorrect[k]++; // If correct answer, add to the array which stores the number of correct answers for that student
            }
        }
    }

    /* Find the highest score */

    int m;

    for (m = 0; m < 4; m++) {
        if (aCorrect[m] > aHighest) // Compare the students number of answers correct for each student to the current highest
            aHighest = aCorrect[m]; // If higher than the current, replace with the new highest
    }

    /* Compute the grade */

    int p;

    for (p = 0; p < 4; p++) {
            if (aCorrect[p] >= (aHighest - 3)) { // Compare the students number of answers correct to the current highest minus 3 (requirement for an A)
                sGrades[p][1] = 'A'; // Store the grade A into the array holding student grades
            }
            else if (aCorrect[p] >= (aHighest - 5)) { // Compare the students number of answers correct to the current highest minus 5 (requirement for a B)
                sGrades[p][1] = 'B'; // Store the grade B into the array holding student grades
            }
            else if (aCorrect[p] >= (aHighest - 7)) { // Compare the students number of answers correct to the current highest minus 7 (requirement for a C)
                sGrades[p][1] = 'C'; // Store the grade C into the array holding student grades
            }
            else if (aCorrect[p] >= (aHighest - 9)) { // Compare the students number of answers correct to the current highest minus 9 (requirement for a D)
                sGrades[p][1] = 'D'; // Store the grade D into the array holding student grades
            }
            else //  The students number of answers correct is below the current highest minus 9 (requirement for a F)
            sGrades[p][1] = 'F'; // Store the grade F into the array holding student grades
    }

}

/* MODULE 4000 */

void printAll() {

    // C = Column
    // R = Row

    /* Print Labels */
    printf("\n\n%-*s", 10, "Name"); // C1 R1
    printf("%-*s", 45, "Answers"); // C2 R1
    printf("%-*s", 20, "Number Correct"); // C3 R1
    printf("%-*s\n", 10, "Grade"); // C4 R1

    /* Print a dashed line */
    int i;
    for (i = 0; i < 80; i++) 
        printf("%c", '-'); // C1 R2

    printf("\n");

    /* Print data */
    int j, k;
    for (j = 0; j < 4; j++) { // For loop to print data on all 4 students
        printf("%-*s", 10, sNames[j]); // C1 - R3, R4, R5, R6
        for (k = 0; k < 19; k++)  // For loop again to print the students answers
            printf("%d ", sAnswers[j][k]);
        printf("%-*d", 7, sAnswers[j][19]); // C2 - R3, R4, R5, R6
        printf("%-*d", 20, aCorrect[j]); // C3 - R3, R4, R5, R6
        printf("%-*c\n", 10, sGrades[j][1]); // C4 - R3, R4, R5, R6
    }

    /* Print Labels */
    printf("\n\n%-*s", 10, "Answer Key"); // C1 R7
    printf("%-*s", 45, ""); // C2 R7
    printf("%s\n", "Highest Score"); // C3 R7

    /* Print a dashed line */
    int l;
    for (l = 0; l < 68; l++)
        printf("%c", '-'); // C1 R8

    printf("\n");

    /* Print Data */
    int m;
    for (m = 0; m < 20; m++) // For loop to print the answer key
        printf("%d ", aKey[m]);

    printf("%-*s", 15, ""); // C3 R9
    printf("%-*d\n\n", 20, aHighest); // C3 R9

}
\$\endgroup\$
  • 5
    \$\begingroup\$ Not teaching C99 in 2015 is really sad. \$\endgroup\$ – 200_success Nov 12 '15 at 4:07
  • 1
    \$\begingroup\$ This would make a good codegolf challenge \$\endgroup\$ – SztupY Nov 12 '15 at 12:38
  • \$\begingroup\$ @SztupY You think? \$\endgroup\$ – Insane Nov 13 '15 at 9:03
10
\$\begingroup\$

Lint

Some of the more obvious simple things that the compiler didn't complain about:

  • Why are you hard-coding 4 and 20 all over the place?
  • It's conventional to order the functions (i.e. main() at the bottom) such that you don't need forward declarations.
  • Commenting every line is very distracting, and more harmful than helpful. A lot of the code (e.g. printf() and scanf()) is obvious, and you're making me read everything twice. Note that // comments were introduced in C99, so your code is neither C89-compliant, nor is it using C99 features, which puts it in a grey zone.

    Your /* */ comments, on the other hand, are somewhat helpful. Keep those.

    More importantly, instead of the meaningless /* Module 1000 */ comments, tell us what global variables the function uses. Documenting those dependencies and side-effects is crucial.

Data structures

First of all, I'm glad to see that you broke down the work into four helper functions. It would be even better if you passed parameters instead of using global variables. Passing arrays in C is somewhat tricky, though, so I wouldn't fault a beginner for avoiding that hurdle.

Have you learned about structs yet? If so, I would prefer to see an array of four struct student_records than to have the information dispersed into various four-element arrays.

As noted in the compiler warnings, sGrades[n][1] involves out-of-bounds access. You don't need a char[][1] if all you need to store is one byte — just a one-dimensional char[] will do.

You don't need initialize(). You can just use an initializer. In fact, global variables are automatically initialized to zero. (Better to be safe than sorry, though!)

Input / output

scanf() is a tricky function to use correctly. In particular, this line

scanf("%s", sNames[i]); // Scan in the students name

is problematic. Things that could go wrong:

  • Buffer overflow if the input exceeds 19 bytes.
  • The whole program malfunctions if the user enters a first name and a last name.
  • You didn't check for end-of-file (CtrlZ on Windows), which would lead to sNames[i] not being written to at all.

In that situation, to read a line of text up to a certain length limit, I suggest using fgets() instead.

Printing could be better as well. For example, for /* Print Labels */, I suggest

printf("%-10s%-45s%-20s%-10s\n", "Name", "Answers", "Number Correct", "Grade");

printf("%c", '-'); is overkill; putchar('-') will do.

Please do not omit "optional" braces like that in your for-loops — you will someday contribute to a coding accident, perhaps a costly one. Just put the braces in, or, if you really feel the urge, write one-liner blocks.

\$\endgroup\$
  • \$\begingroup\$ Thank you, this is helpful. I actually have to use function prototypes to get full marks which is why I'm doing it but I didn't know it was conventional to put main at the bottom. I haven't learned about struct's yet but I will see what I can do with those instead. It's good to know I don't need initialize() also, but again I was forced to do it. Thanks for all the input. \$\endgroup\$ – Insane Nov 12 '15 at 5:23
  • \$\begingroup\$ Also I forgot to mention the reason there are so many comments is because this particular assignment requires "full internal documentation". It's stupid but if I don't I won't get full credit. Trust me I won't be so dang annoying the next time around (and every other time). :P \$\endgroup\$ – Insane Nov 12 '15 at 5:28
  • 3
    \$\begingroup\$ If your instructor specifically requires line-by-line comments, Code Review members will give him/her an earful of complaints. As I said, your /* comments */ are helpful, and should be sufficient. The // comments on every line are really bad practice. \$\endgroup\$ – 200_success Nov 12 '15 at 5:31
  • \$\begingroup\$ It's a gray area honestly if that much is required, but I do know it's definite overkill and bad practice in the real world. I just don't want to get marked off, especially because the teaching assistants are grading, hah. And the whole /* Module 0000 */ is also required to go along with my 'structure chart'. \$\endgroup\$ – Insane Nov 12 '15 at 5:33
  • \$\begingroup\$ @Insane, Would it be an alternative to use /* Module 1000 - Initialize */? That way you can keep the structure number, but also add some information to it. \$\endgroup\$ – holroy Nov 12 '15 at 12:05
5
\$\begingroup\$

The first step is to get your compiler to help review your code. Compiling with warnings…

grade.c:1:33: warning: // comments are not allowed in this language [-Wcomment]
#define _CRT_SECURE_NO_WARNINGS // Required for Visual Studio 2015
                                ^
grade.c:55:26: warning: data argument not used by format string
      [-Wformat-extra-args]
        printf("Name: ", (i + 1)); // Prompt for the students name
               ~~~~~~~~  ^
grade.c:109:17: warning: array index 1 is past the end of the array (which
      contains 1 element) [-Warray-bounds]
                sGrades[p][1] = 'A'; // Store the grade A into the arra...
                ^          ~
grade.c:14:1: note: array 'sGrades' declared here
char sGrades[NUM_STUDENTS][1]; // Global Array for storing the grades o...
^
grade.c:112:17: warning: array index 1 is past the end of the array (which
      contains 1 element) [-Warray-bounds]
                sGrades[p][1] = 'B'; // Store the grade B into the arra...
                ^          ~
grade.c:14:1: note: array 'sGrades' declared here
char sGrades[NUM_STUDENTS][1]; // Global Array for storing the grades o...
^
… (etc.)
\$\endgroup\$
  • \$\begingroup\$ I don't get any of those warnings when compiling on my university's computer system, and I only get the warning: data argument not used by format string in Visual Studio (which I have fixed). Could I ask why this might be? \$\endgroup\$ – Insane Nov 12 '15 at 4:49
  • 2
    \$\begingroup\$ Oh. I forgot that you're using a crappy compiler. My condolences. =) \$\endgroup\$ – 200_success Nov 12 '15 at 4:50
  • \$\begingroup\$ I updated my comment. I actually don't see any of those except for the 2nd one in Visual Studio, which I fixed. It was a typo; should've been simply printf("Name: "); \$\endgroup\$ – Insane Nov 12 '15 at 4:50
  • \$\begingroup\$ I also don't know what note: array 'sGrades' declared here or note: array 'sGrades' declared here means. And I'm not sure why it's saying I can't use a comment after my first line, that doesn't make sense. Also could you tell me what compiler you used to get these warnings so I can try to fix them? Thank you :) \$\endgroup\$ – Insane Nov 12 '15 at 5:07
  • 1
    \$\begingroup\$ I used clang -Wall -std=c89, with LLVM 7.0. gcc 4.8 is stricter: it treats the // comments as errors in C89 mode, but it neglects to point out the problems quoted above. The sGrades message is telling you that the last element you can use is sGrades[p][0], not sGrades[p][1]. \$\endgroup\$ – 200_success Nov 12 '15 at 5:24

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.