3
\$\begingroup\$

I'm still a beginner and I wanted to ask if I'm doing my code right? The comments are the tasks that I need to do for each function.

#include<stdio.h>
#include<string.h>
#include<conio.h>
#define max 5

typedef struct
{
  char name[50];
  float score;
  char grade;
}Student;

void Initialize(Student s[]);
void InputScore(Student s[]);
void AssignGrade(Student s[]); 
void DisplayAll(Student s[]);

int main( )
{

  Student student[max];

  Initialize(student);

  InputScore(student);

  AssignGrade(student);

  DisplayAll(student);

  getch();

}

void Initialize(Student s[])   // assign a blank space to name, 0.0 to score, and blank space to grade. Do this for elements in the array.
{
  int i;

  for(i=0;i<max;i++)
  {
    s[i].name[50] = ' ';    
    s[i].score = 0.0;
    s[i].grade = ' ';
  }
}

void InputScore(Student s[])   // ask for name and score from user. All of the array should be filled up (use a loop)
{
  int i;

  for(i=0;i<max;i++)
  {
    fflush(stdin);
    printf("ENTER STUDENT INFORMATION:\n");
    printf("Name:");
        gets(s[i].name);
    printf("Score:");
        scanf("%f",&s[i].score);
    printf("\n");
  }

}

void AssignGrade(Student s[])  // based on each student score, assign a grade. Let your program do the assigning of the grade  of each student in the array. 
{
  int i;

  for(i=0;i<max;i++)
  {
    if(s[i].score >= 0.000000 && s[i].score <= 59.999999)
    { 
        s[i].grade = 'F'; 
    } 
    else if(s[i].score >= 60.000000 && s[i].score <= 69.999999) 
    { 
        s[i].grade = 'D'; 
    } 
    else if(s[i].score >= 70.000000 && s[i].score <= 79.999999) 
    { 
        s[i].grade = 'C'; 
    } 
    else if(s[i].score >= 80.000000 && s[i].score <= 89.999999)     
    { 
        s[i].grade = 'B'; 
    } 
    else if (s[i].score >= 90.000000 && s[i].score <= 100) 
    { 
        s[i].grade = 'A'; 
    }       
  }
}

void DisplayAll(Student s[])  // display all records in the array
{
  int i;

  for(i = 0;i<40;i++)
  {
  printf("_");printf("\n");
  }

  for(i = 0;i < max;i++)
  {
    printf("\nName: %s",s[i].name);
    printf("\nScore: %.1f",s[i].score);
    printf("\nGrade: %c",s[i].grade);
    printf("\n");
  }  
}
\$\endgroup\$
0

4 Answers 4

8
\$\begingroup\$

Undefined Behavior

s[i].name[50] = ' ';

This accesses outside of the array bounds.

fflush(stdin);

This is undefined behavior.

Undefined behavior allows the implementation to interpret the code in an arbitrarily nonsensical or bizarre manner. The worst part is it might work some of the time, tricking you into the belief that very much incorrect code is correct. Avoid it at all costs.

gets()

Never, repeat never, use gets(). This function is no longer standard C and lacks any protection against buffer overflow.

gets(s[i].name);

Replace with:

fgets(s[i].name, sizeof(s[i].name), stdin);

Using gets() is like invoking undefined behavior. It is really just asking for trouble.

Initialize()

Why manually set properties to blank? Why use space to represent blank? Conventionally, the null byte '\0' represents blank.

Just use:

memset(&s[i], 0, sizeof(Student));

Inconsistent indentation and formatting

// fflush(stdin);
printf("ENTER STUDENT INFORMATION:\n");
printf("Name:");
    gets(s[i].name);
printf("Score:");
    scanf("%f",&s[i].score);
printf("\n");

These functions are in the same block, so they should be indented the same.

Redundant printf()

Replace printf("_");printf("\n"); with printf("_\n");.

printf() vs puts()

Better yet, replace printf() with puts() unless you are printing formatted output.

<= vs <

Consider using s[i].score < 60 instead of s[i].score <= 59.999999, s[i].score < 70 instead of s[i].score <= 69.999999, etc.

Redundant Test

The else if chain ensures that the subsequent if statements failed, i.e., their condition was false. As such, testing if s[i].score => 60 after already testing s[i].score < 60 in a previous if statement followed by an else if is redundant.

\$\endgroup\$
7
  • \$\begingroup\$ fgets(s[i].name, 50, stdin); is better as: fgets (s[i].name, sizeof (s[i].name, stdin);. The redundant printf() should be replaced with puts() as we do not need to formatting feature here, and the call to memset() is not required; s[i].name = { 0 }; would do the job. \$\endgroup\$
    – Harith
    Mar 5, 2023 at 6:11
  • \$\begingroup\$ @Haris Are you sure? I thought you couldn't just assign arrays like that. \$\endgroup\$ Mar 5, 2023 at 6:19
  • \$\begingroup\$ Mm, the code is misleading. OP has declared a local variable, and is then passing it an as argument to functions. Yes, the call to memset() is correct (albeit unnecessary). \$\endgroup\$
    – Harith
    Mar 5, 2023 at 6:23
  • 1
    \$\begingroup\$ @Haris strcpy(s[i].name, " ") also works ok, although the array assignment to 0 is probably best. \$\endgroup\$
    – pacmaninbw
    Mar 5, 2023 at 14:46
  • 2
    \$\begingroup\$ @Haris Global variable -> zero initialized. Local variable -> indeterminately initialized. If they are going to be using a global variable anyway then the Initialize() function altogether is superfluous. \$\endgroup\$ Mar 5, 2023 at 16:30
3
\$\begingroup\$

General Observations

The code might be more readable if the indentation was 4 spaces rather than only 2 spaces. Many programmers use a tab for indentation. It is also common to have spaces between the operators and operands in C statements, the following is more readable than what is currently in the code:

  for (i = 0; i < max; i++)

current code:

  for(i=0;i<max;i++)

The code is not portable because it uses conio.h and getch(), int c = getchar(); is portable and is part of stdio.h.

It also isn't clear why getch() is called in main() since the code is finished at this point.

main() Declaration

While the main() declaration does compile in this case, it would be better to indicate that there are no comments with the following declaration:

int main(void)

There are really only 2 ways to declare main(), either

int main(int argc, char* argv[])

or the first example.

Code Organization

Function prototypes are very useful in large programs that contain multiple source files, and that in case they will be in header files. In a single file program like this it is better to put the main() function at the bottom of the file and all the functions that get used in the proper order above main(). Keep in mind that every line of code written is another line of code where a bug can crawl into the code.

Magic Numbers

There are a lot of Magic Numbers in the code, it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier. The numbers that should be converted to symbolic constants are 50 (NAME_SIZE), 0.0 (FAIL_BASE), 5.999999 (FAIL_MAX), 60.000000, 69.999999, etc.

There is already one symbolic constant in the code, that is max. It is customary to make symbolic constants in C all capitals, this helps identify the symbolic constants when reading the code.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

The Variable i in the For Loops

Depending on the version of C you are using you should be able to declare and initialize the variable i in the for loop. This is true in the more recent versions of C, you may have to go back to C89 to not be able to declare i in the for loop.

void Initialize(Student s[])   // assign a blank space to name, 0.0 to score, and blank space to grade. Do this for elements in the array.
{
  for(int i = 0; i < max; i++)
  {
    s[i].name[50] = ' ';    
    s[i].score = 0.0;
    s[i].grade = ' ';
  }
}

The Initialize Function

It is somewhat surpising that the code works as expected, The assignment of the space to the name field could be causing UB (Undefined Behavior). Since the name field is only 50 characters wide and arrays in C start at 0 the assignment to name[50] is out of range and is probably affecting the the other fields in the structure.

The last valid location in name is name[49].

If you are trying to set the entire array to a blank space, the best way to do it would be to use memset(void *dest, int value, size_t Length).

    memset(s[i].name, ` `, NAME_SIZE);  

To initialize the string to a single blank space

    strcpy(s[i].name, " ");  

It might also be better if the Initialize function only modified a single struct and was called from void InputScore(Student s[]).

The AssignGrade Function

This function can be somewhat simplified:

#define D_MINIMUM  60.0
#define C_MINIMUM  70.0
#define B_MINIMUM  80.0
#define A_MINIMUM  90.0
#define A_MAXIMUM  100.0

void AssignGrade(Student s[])  // based on each student score, assign a grade. Let your program do the assigning of the grade  of each student in the array.
{
  for(size_t i = 0; i < max; i++)
  {
    if(s[i].score <= D_MINIMUM)
    {
        s[i].grade = 'F';
    }
    else if(s[i].score <= C_MINIMUM)
    {
        s[i].grade = 'D';
    }
    else if(s[i].score <= B_MINIMUM)
    {
        s[i].grade = 'C';
    }
    else if(s[i].score <= A_MINIMUM)
    {
        s[i].grade = 'B';
    }
    else if (s[i].score <= A_MAXIMUM)
    {
        s[i].grade = 'A';
    }
  }
}

The code doesn't need the full range since you have already checked the lower range.

The DisplayAll Function

It isn't clear why there are 2 printf() statements in the first loop, it could be only one statement.

void DisplayAll(Student s[])  // display all records in the array
{
  size_t i;

  for(i = 0;i<40;i++)
  {
    printf("_\n");
  }

  for(i = 0;i < max;i++)
  {
    printf("\nName: %s",s[i].name);
    printf("\nScore: %.1f",s[i].score);
    printf("\nGrade: %c",s[i].grade);
    printf("\n");
  }
}

There is also a indentation problem on the line with the 2 printf() statements. Also there should always be only one statement per line.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ You could also replace the four printf() calls in the second loop into a single one. With string literal concatenation and proper indentation it could even look nice. \$\endgroup\$
    – G. Sliepen
    Mar 5, 2023 at 20:37
  • \$\begingroup\$ My 5 cents: I would create a method to get Grade by score: char GetGradeByScore(float score); It would be more readable. Also, it would be nice to define Grages: #define GradeA 'A' \$\endgroup\$
    – Dmitry
    Mar 5, 2023 at 22:28
  • \$\begingroup\$ You can't say "many programmers use tab for indentation" without also saying "many coding standards forbid tabs". There may be arguments for both, Including this one, I'm not sure we should pick sides here. \$\endgroup\$
    – AShelly
    Mar 5, 2023 at 23:18
  • \$\begingroup\$ @AShelly I use 4 spaces. \$\endgroup\$
    – pacmaninbw
    Mar 6, 2023 at 2:29
1
\$\begingroup\$

Additional thoughts to add about void AssignGrade(Student s[]):

Avoid the hole

As is, OP's code "works" with && s[i].score <= 59.999999 ... s[i].score >= 60.000000 as there are no float between 59.999999 and 60.000000. Also applies with larger xx.999999 values.

Yet there is a float (9.99999905...) between 9.999999 and 10.000000.

Should code change float score; to double score;, then there are many possible score between x.999999 and x+1.0.

Simplify code and use a single ended test.

Table driven

Rather than many lines of code, use a table to drive the grade assignment.

Use float constants

With float, use float constants. Sometimes it make a difference.

General C coding: When sizing and indexing, consider size_t vs. other types such as int to handle all array sizes.

Advanced: Not-a-number

float can have a value of not-a-number. Use this to cope the initial state when there is no .score or other exceptional cases.


typedef struct {
  float score_min;  // Only one end defines the grade range.
  char grade;
} score_grade;

const score_grade sg_map[] = { //
    {NAN, 'I'}, {-INFINITY, 'F'}, // 
    {60.0f, 'D'}, {70.0f, 'C'}, // 
    {80.0f, 'B'}, {90.0f, 'A'}, //
};

const size_t sg_map_n = sizeof sg_map / sizeof sg_map[0];

void AssignGrade(Student s[]) {
  for (size_t i = 0; i < max; i++) {
    size_t g = sg_map_n;
    while (--g > 0) {
      if (s[i].score >= sg_map[g].score_min) {
        break;
      }
    }
    s[i].grade = sg_map[g].grade;
  }
}

Edge cases

Consider a grade range like xx.1 (e.g. 90.1) and the student has a .score of 90.1. But stored as a float, the closest float is about 90.099990845...f. If code used 90.1, a double, as a test, then code would compare 90.099990845... as less than 90.1 and you would have an angry student. Avoid such troubles and stick to one floating point type.

\$\endgroup\$
0
\$\begingroup\$

I'm just going to look at the AssignGrade() function.

void AssignGrade(Student s[])  // based on each student score, assign a grade. Let your program do the assigning of the grade  of each student in the array. 

This function just assumes that the size of the Student array is stored in the global variable max. While it might be true for this small test program, it is not great coding style. In general, it's better to pass in the size of array arguments and allow the function to handle arbitrary sizes. void AssignGrades(Student s[], int numStudents)

{
  int i;

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

As pointed out elsewhere, for (int i = 0; ... is more modern C. One reason is to limit the scope of the loop variable. Limiting variable scope generally makes programs easier to understand and less error prone.

max is not a great name - it doesn't describe what is it the maximum of, and that name is often used for functions which return the maximum item in a set.

    if(s[i].score >= 0.000000 && s[i].score <= 59.999999)
    { 

There is no difference between specifying 0.0 vs 0.000000. The former implies the latter. Having a decimal point makes it a floating point constant, adding the extra zeros doesn't change the meaning at all.

        s[i].grade = 'F'; 
    } 
    else if(s[i].score >= 60.000000 && s[i].score <= 69.999999) 
    { 
        s[i].grade = 'D'; 
    } 

Other commenters pointed out that the test >= 60.0000000 is redundant, since the previous test is checking for < 59.999999, but it's worse than that - it could actually let cases slip through. Do you know with certainty that a float can't take the value 59.999995 ? Even if you do, what happens when someone decides to make the score field a double instead? Now you've broken this function by changing code in a completely different part of the file. Much better to test each one as score < 60.0, score < 70.0, etc.

    else if(s[i].score >= 70.000000 && s[i].score <= 79.999999) 
    { 
        s[i].grade = 'C'; 
    } 
    else if(s[i].score >= 80.000000 && s[i].score <= 89.999999)     
    { 
        s[i].grade = 'B'; 
    } 
    else if (s[i].score >= 90.000000 && s[i].score <= 100) 
    { 
        s[i].grade = 'A'; 
    }
  }
}

Even if you fix the numeric limits, there are still two cases that slip through. What if score < 0.0 or > 100.0 ? In this case the function returns silently without actually assigning a grade. You need a check for scores out of the expected range. There are a few strategies you can take when you detect this.

  1. You could assign letter values to those ranges - declare that anything over 90 is an A, no matter how far above.
  2. You can print an error message.
  3. You can modify the function signature to return an error code, and let the caller decide how to handle failures - ignore or abort or ask for the numeric to be re-entered.
  4. You can assert that the score must be >= 0.0 and <= 100, and put the responsibility of ensuring that condition on the InputScore method.

Whether or not to implement Option 1 is a question for the "customer". For a program to be correct, it needs to satisfy the expectations of the end user.

I don't generally like Option 2 because it couples logic and interface. The code to get a grade from a number should be independent of how to signal errors. A better design would let you change how you signal the user without touching and of the code that performs fundamental calculations.

Option 3 is reasonable,and generally standard practice. But in many cases, I argue for option 4. The simplest way to add error handling to programs is to scrub input for errors before it gets into the system. If we decide that the only valid values for scores are between 0 and 100, then we can add validity checking code wherever a score is generated (in this case only InputScore), and we don't need to make decisions about how to handle errors anywhere else downstream.

\$\endgroup\$
2
  • \$\begingroup\$ "Much better to test each one as score < 60.0, score < 70.0, etc." is followed by two tests code: else if(s[i].score >= 70.000000 && s[i].score <= 79.999999) ..... Is that intended? \$\endgroup\$ Mar 6, 2023 at 13:35
  • \$\begingroup\$ The code is the OP's original, I just inserted comments inbetwen. \$\endgroup\$
    – AShelly
    Mar 7, 2023 at 0:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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