Finding students with the best grades in a given subject

Question

We have a school assignment, in which the program is to read data of students from stdin, in the following format: LASTNAME FIRSTNAME STUDENT_ID SUBJECT AVERAGE_GRADES. The program has to print out the data of students with the top 3 averages, whose subject is the same as the one given in the first line in a similar format as the input, only leaving out the subject. If two students have the same averages, their data should be in a decreasing lexographical order of their STUDENT_ID.

So I can write the program and it works properly, but the last time during practice my teacher looked at my screen and bragged, that he could do the same problem in about 30-50 lines of code. My question is, can you give me some hints, whether there is any superfluous lines in my code? Is there anything that I overcomplicate, or just could be done in a simpler, more professional way?

Example Input:

CS
Jane Doe 123ABC GI 4.5
Mark Spencer AFG12L GI 4.4
Charlotte Fringe LUK65E CS 4.0
Stephen Wright ART12M CS 2.8
Christine Smith ERTZ12 IE 2.7
Adam Lee ZTI98M GI 3.4
Juliette Johnson ARE74W CS 4.0
Kevin Crane QWER99 IE 2.6
Jessica Rhoades ASDF89 GI 2.5
Mark Mason ACDC12 IE 3.8
Jules Shepard XYC78L CS 4.9

Example Output:

Jules Shepard XYC78L 4.9
Juliette Johnson ARE74W 4.0
Charlotte Fringe LUK65E 4.0

And the code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>


struct student_data {
    char firstname[30];
    char lastname[30];
    char neptun_code[7];
    char subject[10];
    float average;
};

int main()
{
    int i; // outer incrementation counter
    int j; // inner incrementation counter
    int line_num = 0;
    int printed_lines = 0;
    int attr_counter;
    char subject_given[10];
    char *string_buffer = malloc(1000 * sizeof(char));
    char *token_buffer = malloc(1000 * sizeof(char));
    struct student_data student_data_all[1000];
    struct student_data top_three[3];
    struct student_data swap_buffer; // needed for sorting

    //read in lines from stdin in the format of LASTNAME FIRSTNAME STUDENT_ID SUBJECT AVERAGE_GRADES
    while(fgets(string_buffer, 1000, stdin) != NULL && string_buffer[0] != '\n')
    {
    //read in the first line which contains a subject name, then trim the trailing \n, this will be needed for strcmp to work properly at the end of the code
        if(line_num == 0)
            {
                token_buffer = strtok(string_buffer, "\n");
                strcpy(subject_given, token_buffer);
                line_num++;
                continue;
            }
    //fill up the structure array with relevant data using whitespace as a delimiter    
        attr_counter = 0;
        token_buffer = strtok(string_buffer, " ");
        while(token_buffer != NULL)
        {
            if(attr_counter == 0)
                strcpy(student_data_all[line_num - 1].lastname, token_buffer);
            else if(attr_counter == 1)
                strcpy(student_data_all[line_num - 1].firstname, token_buffer);
            else if(attr_counter == 2)
                strcpy(student_data_all[line_num - 1].neptun_code, token_buffer);
            else if(attr_counter == 3)
                strcpy(student_data_all[line_num - 1].subject, token_buffer);
            else if(attr_counter == 4)
                student_data_all[line_num - 1].average = strtod(token_buffer, NULL);

            token_buffer = strtok(NULL, " ");
            attr_counter++;
        }
        line_num++;
    }
    //sort the student_data_all structure in decreasing order of average
    for(i = 0; i < line_num -1; i++)
    {
        for(j = i + 1; j < line_num -1; j++)
        {
            if(student_data_all[i].average < student_data_all[j].average)
            {
                swap_buffer.average = student_data_all[i].average;
                strcpy(swap_buffer.firstname, student_data_all[i].firstname);
                strcpy(swap_buffer.lastname, student_data_all[i].lastname);
                strcpy(swap_buffer.neptun_code, student_data_all[i].neptun_code);
                strcpy(swap_buffer.subject, student_data_all[i].subject);

                student_data_all[i].average = student_data_all[j].average;
                strcpy(student_data_all[i].firstname, student_data_all[j].firstname);
                strcpy(student_data_all[i].lastname, student_data_all[j].lastname);
                strcpy(student_data_all[i].neptun_code, student_data_all[j].neptun_code);
                strcpy(student_data_all[i].subject, student_data_all[j].subject);

                student_data_all[j].average = swap_buffer.average;
                strcpy(student_data_all[j].firstname, swap_buffer.firstname);
                strcpy(student_data_all[j].lastname, swap_buffer.lastname);
                strcpy(student_data_all[j].neptun_code, swap_buffer.neptun_code);
                strcpy(student_data_all[j].subject, swap_buffer.subject);
            }
        }
    }
    //in case two students have the same average, they should be listed in the increasing order of their student id, this conditional sorting is done here
    for(i = 0; i < line_num -1; i++)
    {
        for(j = i + 1; j < line_num -1; j++)
        {
            if(student_data_all[i].average == student_data_all[j].average)
            {
                if(strcmp(student_data_all[i].neptun_code,student_data_all[j].neptun_code) > 0)
                {
                    swap_buffer.average = student_data_all[i].average;
                    strcpy(swap_buffer.firstname, student_data_all[i].firstname);
                    strcpy(swap_buffer.lastname, student_data_all[i].lastname);
                    strcpy(swap_buffer.neptun_code, student_data_all[i].neptun_code);
                    strcpy(swap_buffer.subject, student_data_all[i].subject);

                    student_data_all[i].average = student_data_all[j].average;
                    strcpy(student_data_all[i].firstname, student_data_all[j].firstname);
                    strcpy(student_data_all[i].lastname, student_data_all[j].lastname);
                    strcpy(student_data_all[i].neptun_code, student_data_all[j].neptun_code);
                    strcpy(student_data_all[i].subject, student_data_all[j].subject);

                    student_data_all[j].average = swap_buffer.average;
                    strcpy(student_data_all[j].firstname, swap_buffer.firstname);
                    strcpy(student_data_all[j].lastname, swap_buffer.lastname);
                    strcpy(student_data_all[j].neptun_code, swap_buffer.neptun_code);
                    strcpy(student_data_all[j].subject, swap_buffer.subject);
                }
            }
        }
    }

    //print out three students with the top averages
    for(i = 0; i < line_num - 1; i++)
    {
        if(strcmp(student_data_all[i].subject, subject_given) == 0)
        {
            printf("%s %s %s %.1f\n", student_data_all[i].lastname,student_data_all[i].firstname, student_data_all[i].neptun_code, student_data_all[i].average);
            printed_lines++;    
            if(printed_lines == 3)
                break;
        }
    }

    return 0;
}

Answer 1

The DRY Programming Principle

Ask your teacher if he was referring to the DRY programming principle.

In software engineering, don't repeat yourself (DRY) is a principle of software development aimed at reducing repetition of information of all kinds, especially useful in multi-tier architectures.

This code repeats almost exactly 6 times:

                    student_data_all[j].average = swap_buffer.average;
                    strcpy(student_data_all[j].firstname, swap_buffer.firstname);
                    strcpy(student_data_all[j].lastname, swap_buffer.lastname);
                    strcpy(student_data_all[j].neptun_code, swap_buffer.neptun_code);
                    strcpy(student_data_all[j].subject, swap_buffer.subject);

A good way to reduce the amount of code in the program is to create a function that copies the data from one structure to another. That would would reduce the amount of code in the program by almost 30 lines. Writing it in a function also makes it much easier to write and debug, since it only needs to be debugged once.

Use Symbolic Constants Versus Numeric Constants

In the code there are raw numbers for the first names size, last name size, neptun code size and subject size. It might be better to use symbolic constants rather than raw numbers. An Example:

#define NAME_SIZE 30
#define NEPTUN_CODE_SIZE 7
#define SUBJECT_SIZE 10

struct student_data {
    char firstname[NAME_SIZE];
    char lastname[NAMES_SIZE];
    char neptun_code[NEPTUN_CODE_SIZE];
    char subject[SUBJECT_SIZE];
    float average;
};

Some symbolic constants are included in header files such as BUFSIZ in stdio.h. BUFSIZ might be a good constant to use in the calls to malloc() since it is a system defined buffer size.

Symbolic constants are good because it allows the code to be changed in only one place when array sizes need to be changed. Quite often array sizes are also loop constants so it really helps to have only one place to edit the code. It is also good because it tells anyone reading the code what the number should be used for.

In some cases it might be possible to use enum types as sumbolic constants as well:

enum {
    ATTRIBUTE_COUNT_FIRST, 
    ATTRIBUTE_COUNT_LAST, 
    ATTRIBUTE_COUNT_NEPTUN_CODE,
    ATTRIBUTE_COUNT_SUBJECT,
    ATTRIBUTE_COUNT_GPA
};

Reduce the Complexity of the Functions

The first while loop in main() should probably be it's own function, possibly called get_student_data(). It's generally a good practice to break code up into smaller functions that have simple tasks to do. The get_student_data() function might also be broken up into smaller functions.

The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

Robert C. Martin expresses the principle as follows:
    `A class should have only one reason to change.`

While this is primarily targeted at classes in object oriented languages it applies to functions and subroutines in procedural languages like C as well.

Answer 2

Your two loops can be condensed into a single loop.

for(i = 0; i < line_num -1; i++)
{
    for(j = i + 1; j < line_num -1; j++)
    {
        if(student_data_all[i].average < student_data_all[j].average)
        {
             // SWAP
        }
    }
}
for(i = 0; i < line_num -1; i++)
{
    for(j = i + 1; j < line_num -1; j++)
    {
        if(student_data_all[i].average == student_data_all[j].average)
        {
            if(strcmp(student_data_all[i].neptun_code,student_data_all[j].neptun_code) > 0)
            {
                // SWAP
            }
        }
    }
}

Seems like you are sorting then sorting again.
You only need to sort once; to help with this also define a comparison operator so the code looks neat:

int isLeftStudentSmarter(student_data const* lhs, student_data const* rhs) {
    return    (lhs->average < rhs->average)
           || (lhs->average == rhs->average && strcmp(lhs->neptun_code, rhs->neptun_code) > 0);
}

for (i = 0; i < line_num -1; i++)
{
    for (j = i + 1; j < line_num -1; j++)
    {
        if (isLeftStudentSmarter(&student_data_all[i], &student_data_all[j]))
        {
             // SWAP
        }
    }
}

If you take this a step further you can replace the loop completely by using a standard function:

We need to have a conversion function that is used 

void qsort(&student_data_all[0], line_num - 1, sizeof(student_data),
       isLeftStudentSmarterVoid);

We need to have a conversion function that is used by qsort()

int isLeftStudentSmarterVoid(void const* lhs, void const* rhs) {
    return isLeftStudentSmarter((student_data const*)lhs, (student_data const*)rhs));
}

You can scan a line of text much more easily then manually pulling each section.

// Mark Spencer AFG12L GI 4.4
student_data* current = &student_data_all[line_num - 1];
int count = fscanf(stdin, " %s %s %s %s %f",
                      current->lastname,
                      current->firstname,
                      current->neptun_code,
                      current->subject,
                      &current->average);
fscanf(stdin, "%*[^\n]");  // Read white space up to new line. 
fscanf(stdin, "%*[\n]");   // Read the new line character.
if (count != 5) {
    /* Error State*/
}

Answer 3

Well, your code seems far too verbose. Take a look at <stdio.h>, especially fscanf().

Next, only retain data which is actually useful. The rest, discard it immediately.

So, your code should go:

1. Read the target subject.
2. Read a students records.
3. If he takes the target subject, and is one of the best N read yet, remember it (and drop the worst candidate if there were N remembered already).
4. Repeat at 2 until end of file.
5. Output the records remembered.

Answer 4

Struct copy

Something no one has brought up yet is that your multiple line struct copy code can be done with a simple assignment instead. In other words, this code:

            swap_buffer.average = student_data_all[i].average;
            strcpy(swap_buffer.firstname, student_data_all[i].firstname);
            strcpy(swap_buffer.lastname, student_data_all[i].lastname);
            strcpy(swap_buffer.neptun_code, student_data_all[i].neptun_code);
            strcpy(swap_buffer.subject, student_data_all[i].subject);

can be done like this instead:

            swap_buffer = student_data_all[i];

Use scanf for input

Instead of using strtok in a complicated way:

//fill up the structure array with relevant data using whitespace as a delimiter    
    attr_counter = 0;
    token_buffer = strtok(string_buffer, " ");
    while(token_buffer != NULL)
    {
        if(attr_counter == 0)
            strcpy(student_data_all[line_num - 1].lastname, token_buffer);
        else if(attr_counter == 1)
            strcpy(student_data_all[line_num - 1].firstname, token_buffer);
        else if(attr_counter == 2)
            strcpy(student_data_all[line_num - 1].neptun_code, token_buffer);
        else if(attr_counter == 3)
            strcpy(student_data_all[line_num - 1].subject, token_buffer);
        else if(attr_counter == 4)
            student_data_all[line_num - 1].average = strtod(token_buffer, NULL);

        token_buffer = strtok(NULL, " ");
        attr_counter++;
    }

You could just use scanf like this:

struct student_data *student = &student_data_all[line_num - 1];
scanf("%s %s %s %s %f", student->lastname, student->firstname,
        student->neptun_code, student->subject, &student->average);

Sort should be done once

Currently, you are sorting your list twice because you are sorting on a different criterion each time. Instead of doing that, you should sort once and just make your sorting criterion include both factors. In other words, check the average, and if they are equal then check the student id.

Code rewrite

Here is what a code rewrite would look like. Note that I added a student_cmp() function to compare two students so that you could use qsort() to do the sort instead of your custom bubble sort.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct student_data {
    char firstname[30];
    char lastname[30];
    char neptun_code[7];
    char subject[10];
    float average;
} student_data;

#define MAX_STUDENTS        1000

// Students are sorted in decreasing order of average.  In case two students
// have the same average, they should be listed in the increasing order of
// their student id.
static int student_cmp(const void *p1, const void *p2)
{
    const student_data *st1 = (const student_data *) p1;
    const student_data *st2 = (const student_data *) p2;

    if (st1->average < st2->average ||
            (st1->average == st2->average &&
            strcmp(st1->neptun_code, st2->neptun_code) > 0))
        return 1;
    else
        return -1;
}

int main(void)
{
    int  num_students = 0;
    char subject_given[10];
    student_data student_data_all[MAX_STUDENTS];

    // Read subject from the first line.
    scanf("%s", subject_given);

    // Each subsequent line holds information on one student.
    for (num_students = 0; num_students < MAX_STUDENTS; num_students++) {
        struct student_data *student = &student_data_all[num_students];
        int ret = scanf("%s %s %s %s %f", student->lastname,
                student->firstname, student->neptun_code, student->subject,
                &student->average);
        if (ret != 5)
            break;
    }

    // Sort the student_data_all structure in decreasing order of average.
    // In case two students have the same average, they should be listed in
    // the increasing order of their student id.
    for (int i = 0; i < num_students; i++) {
        for (int j = i + 1; j < num_students; j++) {
            if (student_cmp(&student_data_all[i], &student_data_all[j]) > 0) {
                struct student_data swap_buffer = student_data_all[i];
                student_data_all[i] = student_data_all[j];
                student_data_all[j] = swap_buffer;
            }
        }
    }

    /* You could replace the above sort with this:
    qsort(student_data_all, num_students, sizeof(student_data_all[0]),
            student_cmp);
    */

    // Print out three students with the top averages.
    int printed_lines = 0;
    for (int i = 0; i < num_students; i++) {
        if(strcmp(student_data_all[i].subject, subject_given) == 0) {
            printf("%s %s %s %.1f\n", student_data_all[i].lastname,
                    student_data_all[i].firstname,
                    student_data_all[i].neptun_code,
                    student_data_all[i].average);
            printed_lines++;
            if (printed_lines == 3)
                break;
        }
    }

    return 0;
}

Note that you can make your code even smaller by using qsort() instead of writing your own sort (see other people's answers for an example of that).