Student sorting based on criteria

Question

This code takes id, name and marks of 3 subjects for 5 students from user base, calculates the total of 3 subjects per student, and then takes a criteria level with which to sort; level 1 sorting does sorting based on 1 criteria only, while level 2 will sort based on criteria 1 and if similar occurs (like if select level 1 criteria marks and if marks of two student are same) sorting of that pair will be done based on criteria 2, if either user have selected level 2 and criteria 2.

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


struct student
{ 
int id;
 int sub[3];
int total;
char name;
}s[5];
void sort1(struct student *[],int,int,int);
main()
{       int i,j,ch,c1,c2,t;
    char choice;
for(i=0;i<5;i++)
{
    s[i].total=0;
printf("Enter the id of s[%d]\n",i+1);
scanf("%d",&s[i].id);
printf("Enter the name:\n");
scanf("%s",&s[i].name);
    for(j=0;j<3;j++)
    {
    printf("Enter the marks of sub[%d] of s[%d]\n",j+1,i+1);
    scanf("%d",&s[i].sub[j]);
    s[i].total+=s[i].sub[j];
    }
}
struct student *p[5];
for(i=0;i<5;i++)
{
    p[i]=&s[i];
}
printf("You wanna sort or not?\n");
scanf("%c",&choice);
if((choice == 'y') || (choice == 'Y'))
{   
    printf("Which level sorting 1 or 2 ?\n");
    scanf("%d",&ch);
    if(ch==2)
    {   
        t=1;
        printf("Criteria:\t1)By name\n2)By Id\n3)By total\nEnter level1 &level 2criteria:\n");
        scanf("%d%d",&c1,&c2);
        sort1(p,t,c1,c2);
    }
    else
    {   t=0;
        printf("Enter the level 1 criteria:\n");
        scanf("%d",&c1);
        sort1(p,t,c1,10);
    }
}
}
void sort1(struct student *p[5],int t,int cri1,int cri2)
    {
int i,j;

struct student *pt;



for(i=0;i<5;i++)
{
    for(j=0;j<5;j++)
    {
        if((t==1)&&(cri1==1) && (strcmp(&p[i]->name,&p[i+j]->name)>0))
        {
            pt=p[i];
            p[i]=p[i+j];
            p[i+j]=pt;  
        }
        else if((t==1) &&(cri2==2)&& (strcmp(&p[i]->name,&p[i+j]->name)==0))
        {   
            if(p[i]->id<p[i+j]->id)
            {       
                pt=p[i];
                p[i]=p[i+j];
                p[i+j]=pt;
            }
        }
        else if((t==1)&&(cri2==3)&&(strcmp(&p[i]->name,&p[i+j]->name)==0))
        {
            if(p[i]->total<p[i+j]->total)
            {
                pt=p[i];
                p[i]=p[i+j];
                p[i+j]=pt;
            }
        }
        else if((t==0)&&(cri1==1) && (strcmp(&p[i]->name,&p[i+j]->name)>0))
        {
            pt=p[i];
            p[i]=p[i+j];
            p[i+j]=pt;  
        }
        else if((t==0) &&(cri1==2)&& (strcmp(&p[i]->name,&p[i+j]->name)>0))
        {   
            if(p[i]->id<p[i+j]->id)
            {       
                pt=p[i];
                p[i]=p[i+j];
                p[i+j]=pt;
            }
        }
        else if((t==0)&&(cri1==3)&&(strcmp(&p[i]->name,&p[i+j]->name)>0))
        {
            if(p[i]->total<p[i+j]->total)
            {
                pt=p[i];
                p[i]=p[i+j];
                p[i+j]=pt;
            }
        }


    }
}
for(i=0;i<5;i++)
{
    printf("Id\tName\tS1\tS2\tS3\tTotal\n\n%d\t%s\%d\t%d\t%d\t%d\n",p[i]->id,p[i]->name,p[i]->sub[0],p[i]->sub[1],p[i]->sub[2],p[i]->total);
}
}

Answer 1

indentation

The most obvious weirdness about your code is the inconsistent indentation. Indeed I was convinced that there was a missing } at the end until I scanned the whole thing much more carefully. Indent everything within a function one level and add additional levels at each new nested statement. (user1118321 has generously provided a version with corrected indentation)

long lines

Very long lines are hard to read. This one, requires me to scroll to read the whole thing. So I can't even see the whole function call all-at-once.

    printf("Id\tName\tS1\tS2\tS3\tTotal\n\n%d\t%s\%d\t%d\t%d\t%d\n",p[i]->id,p[i]->name,p[i]->sub[0],p[i]->sub[1],p[i]->sub[2],p[i]->total);

You can fold this nicely over multiple lines.

    printf("Id\tName\tS1\tS2\tS3\tTotal\n\n%d\t%s\%d\t%d\t%d\t%d\n",
           p[i]->id,p[i]->name,
           p[i]->sub[0],p[i]->sub[1],p[i]->sub[2],
           p[i]->total);

You can even break-up long strings and let the compiler concatenate them for you.

    printf("Id\tName\t" "S1\tS2\tS3\t" "Total\n\n"
           "%d\t%s\t"   "%d\t%d\t%d\t" "%d\n",
           p[i]->id,p[i]->name,
           p[i]->sub[0],p[i]->sub[1],p[i]->sub[2],
           p[i]->total);

Incidentally, I found a missing "t" in there while doing this.

index origin

For output you add 1 to the index, but for input you do not subtract 1. This is bizarre. It would be more natural in a C program to use 0 throughout. The first element is element 0. If you need to, you can document it for your users or even announce it at program startup.

interactive/batch

A more useful program would be one that operates on data-files and accepts command-line arguments as instructions. This saves you lots of typing as you test your program. You can type all the data just once and run your program many times. You can test different options by writing a shell-script or batch-file which runs the program different ways.

You could define a text-representation of your data. A common way is to use : as a separator

Student,Lastname:25:37:44:55
Student,Noname:0:0:0:99
Studentka,Foxy:101:110:150:1000

Then your program could accept the filename and a code or option as an argument.

int main(int argc, char **argv){
    if (argc < 3) {
        printf("too few arguments\n"
               "usage: %s filename option\n"
               "where option is 0 1 2 or 3\n");
    }

    struct student *data = loadfile(argv[1]);
    perform_sort(data, strtol(argv[2],NULL,10));
    print_data(data);
}

It would be good to separate the different parts into different functions, but this is easier to see when you try to make it work with files. You could also accept - as a special file-name that means stdin, and still run your program interactively. You can trigger an End-Of-File condition with ctrl-D (Unix, OSX) or ctrl-Z (Windows) to tell your program where the data ends. This is a common behavior for command-line utilities.

dynamically resizing arrays

In order to approach what I'm describing here, it would be good to build the student database dynamically.

student *data = NULL;
int data_n = 0;

data = malloc((data_n=10) * sizeof *data);

A couple of things to note. I assign the size and allocate the array in the same statement. I don't know if everybody does this, but it helps me make sure that the two variables always change together. It would be good to check the result of malloc for a NULL and do something sensible such as exit(1), but for very simple programs you can usually get away with just assuming malloc will return the memory. This is a fragile behavior and the program is not correct per se, but memory-management is a complicated subject to learn, so it's also good to keep things simple until there is a need to make them complicated.

So what happens if you're reading line number 11? Every time the loadfile function will read a line, it will first check that i < data_n and if not, then it must first

data = realloc(data, (data_n *= 2) * sizeof *data);

to make more room. When you finish reading data, you should make sure there is still some room (i < data_n) and then add a sentinel record to indicate the end of data for the other functions. This could be a student with the name "\0" perhaps.

char ... pointer?

char name;

//...

scanf("%s",&s[i].name);

Unless the name is 0 characters long, that's not enough memory for a name. You could set a maximum size, which is perhaps the easiest way.

char name[100];

Or you could use a pointer to a string, but you must allocate memory for it, and for that it is still easiest to use a fixed-size buffer and then use the POSIX function strdup to make a dynamic copy of the string.

Answer 2

In addition to the other excellent analyses, I have a few suggestions.

Whitespace

I recommend using more whitespace in your code. It's difficult to read as it currently stands. I generally put spaces between operators and operands. So a for loop like this:

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

ends up looking like this:

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

I also add spaces after commas, for example in a list of parameters in a function call. So instead of:

sort1(p,t,c1,c2);

I'd write:

sort1(p, t, c1, c2);

Naming

Your naming starts off pretty good with the student structure. But it kind of loses momentum after that. First, I'd recommend not abbreviating already short words like 'subject'.

Next, loop control variables are often i and j as you've used, so I can live with that, but you really could do better. You're first loop could look something like this:

main()
{
    int studentNum;
    for (studentNum = 0; studentNum < 5; studentNum++)
    {
        s[ studentNum ].total = 0;

        printf("Enter the id of s[%d]\n", studentNum + 1);
        scanf("%d", &s[ studentNum ].id);

        print("Enter the name:\n");
        scanf("%s", &s[ studentNum ].name);

        int subjectNum;
        for (subjectNum = 0; subjectNum < 3; subjectNum++)
        {
            printf("Enter the marks of sub[%d] of s[%d]\n", subjectNum + 1, studentNum + 1);
            scanf("%d", &s[ studentNum ].sub[ subjectNum ]);
            s [ studentNum ].total += s [ studentNum ].sub [ subjectNum ];
        }
    }
...

It becomes a lot more clear what the variables mean.

Functions

Breaking your program into functions has a lot of benefits. You can reuse functions later if you need to do the same task with different pieces of data. It also improves the readability of the code and its maintainability. You've done that with the sort1() function, which is great. You should also do it for filling out the array. I'd move the entire first loop into its own function named something like getStudentRecords() or something of that sort.

Usability

A common issue that users have with computer programs is that they're confusing to use because the interface doesn't make sense to them. There are technical words used or specific orders that data must be entered. I'd recommend rewording your prompts.

Instead of:

printf("Enter the id of s[%d]\n",i+1);

I recommend:

printf("Enter the id of student %d\n", i + 1);

Likewise with this line:

printf("Enter the marks of sub[%d] of s[%d]\n",j+1,i+1);

should be something like this:

printf("Enter the score of subject %d for student %d", j + 1, i + 1);

I suggest using "score" to give the user a clue that you're program is expecting a number value and not a letter grade such as "A" or "B-". You could be even more explicit if you felt like it.

EDIT:

Magic Numbers

One thing I forgot to mention - this code contains a lot of what we refer to as "magic numbers". For example there are 5 student records, declared as:

struct Student{
...
} s [5];

Then, whenever the code needs to access the array, the for loops do this:

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

If you ever have a need to change the value from 5 to 10 or more, you'll end up needing to change it not only in the declaration of s, but in every for loop you use.

You should declare some const values and use those instead of magic numbers. Something like this:

const int kNumStudents = 5;
const int kNumSubjects = 3;

and then when you need to use them, you'd do this:

struct Student {
    int id;
    int subject[kNumSubjects];
    int total;
    char name;
}s[kNumStudents];

And your for loops will look like this:

for (i = 0; i < kNumStudents; i++)
{
    ...
    for (j = 0; j < kNumSubjects; j++)
    {
        ...
    }
}