1

I am trying to make a program that will arrange my structures by people ids. I used long double because IDs are 20 digits long. For example if I introduce 3 persons:

1.Alex Alex / id = 219(...)
2.John John / id = 200(...)
3.Robert Robert / id = 199(...)

I want my program to rearrange so Robert comes first, John second and Alex third. I am having a problem on "for" structure - I don't exactly know how to swap two structures since I have chars and ints combined.

typedef struct 
{
    char name;
    char prename;
    long double id;
    int j;
} PERSON;

int main() 
{
    int n,i,j;
    printf ("How many people = ");
    scanf("%d", &n);
    PERSON v[n];
    for(i=1;i<=n;i++)
    {
        printf("For person number nr. %d\n", i);
        printf("name = ");
        scanf("%s", &v[i].name);
        printf("Prename = ");
        scanf("%s", &v[i].prename);
        printf("id = ");
        scanf("%d", &v[i].id);
    }

    for(int i=0; i<n; i++)
    {
        for(int j=0; j<n-1; j++)
        {
            if( v[i].id > v[j+1].id )
            {
                int temp = v[j].id;
                char temp2[100];
                char temp3[100];

                strcpy(v[j].prename,temp3);
                strcpy(v[j].name,temp2);
                v[j].id = v[j+1].id;
                v[j+1].id = temp;
            }
        }
    }

    return;
}
Michael Dorgan
  • 12,453
  • 3
  • 31
  • 61
Calin Chaly
  • 59
  • 1
  • 7
  • You can swap a `struct` like that exactly as you would swap an integer type. Aside: better to use a string for the ID, because not all C implementations support the `long double` type. – Weather Vane Nov 30 '18 at 17:47
  • The `sorting` looks like a `bubble sort`. For that, your internal `for` (j) need to start from `i` and not from `0`. The logic is that, for each element on the first `for`, you keep checking if it is greater than the element on the right side and if true, switch. – wendelbsilva Nov 30 '18 at 17:51
  • A list should be better for your case, you can directly insert in sorted order. – Ôrel Nov 30 '18 at 18:01
  • What's a list in the land of C? :) Seriously though, placing the sort into its own function so you can swap it out later would be a good idea. – Michael Dorgan Nov 30 '18 at 18:15

2 Answers2

3

The main problem with your code is here:

typedef struct
    char name;      <--- just a char! no room for a string
    char prename;   <--- just a char! no room for a string
    long double id;
     int j;
    } PERSON;

You need to make these arrays so that they can hold the names.

Like:

#define MAX_NAME_LEN 100

typedef struct {
    char name[MAX_NAME_LEN];
    char prename[MAX_NAME_LEN];
    long double id;
    int j;
} PERSON;

Besides that, you should always check the value returned by scanf and never do scanf("%s", &v[i].prename); as it may lead to buffer overflow. Instead do scanf("%99s", v[i].prename); but even better use fgets

And for sorting... just use qsort - see http://man7.org/linux/man-pages/man3/qsort.3.html

BTW: A long double can't be scan'ed using scanf("%d", &v[i].id); - %d is for integers. That said, I doubt you want to use long double. You probably want some interger type - maybe long long unsigned or uint64_t

Support Ukraine
  • 42,271
  • 4
  • 38
  • 63
  • long double for id hashing is also fun as it may not store the digit he wants directly. Funny that a long long stores 19 digits perfectly :/. I wonder if he needs to do some simple BIG MATH here to store the digits he wants instead. – Michael Dorgan Nov 30 '18 at 18:04
  • @MichaelDorgan True... and it's read by `scanf("%d", &v[i].id);` which is another bug – Support Ukraine Nov 30 '18 at 18:07
  • My security side loves putting a raw scanf input value into a VLA. What could go wrong? My god C is a walking booby trap for this crap. – Michael Dorgan Nov 30 '18 at 18:22
3

Ok, because there is so many things you've done here that are not obvious to a new C developer, I want to point them out to help you learn:

typedef struct 
{
    // These need to be arrays if they are to be strings
    // Preferably using a constant for size or dynamic
    // if you want them to be variable sized to match input.
    char name;
    char prename;
    // Do not use floating point numbers to represent integer values.  
    // IRL, you'd use a library, but here, you may want to use an array of 
    // some sort of integer type instead.
    long double id;  

    // This is a really poor name for a struct variable and probably shouldn't be here.
    int j;
} PERSON;

int main() 
{
    int n,i,j;
    printf ("How many people = ");
    // Dropping raw output from scanf into a memory allocation is crazy dangerous.
    // At least error check the results to be sure it is meaningful.
    scanf("%d", &n);
    // This is a variable length array and often is not supported directly.
    // You probably want to use a malloc()/free() pair to handle this.
    // (Update: VLA is now part of newer standards, but are not safe because they cannot fail gracefully on out of memory.)
    PERSON v[n];

    // Anytime in C I see an array start at 1 and use <= for condition, I get very nervous because
    // it tends to lead to indexing errors.
    for(i=1;i<=n;i++)
    {
        printf("For person number nr. %d\n", i);
        printf("name = ");

        // Oops - and this is why.  You just skipped the first entry at 0
        // and will overwrite memory on the last loop.
        // Also, scanf into a string without a length can blow up memory...
        scanf("%s", &v[i].name);
        printf("Prename = ");
        // Ditto
        scanf("%s", &v[i].prename);
        printf("id = ");
        // Ditto - worse because you've crossed your types - %d doesn't go into a long double.
        scanf("%d", &v[i].id);
    }

    // Should be its own function to make it easier to swap later to a better sort.
    for(int i=0; i<n; i++)
    {
        // Bubble sort usually wants j=i here.
        for(int j=0; j<n-1; j++)
        {
            if( v[i].id > v[j+1].id )
            {
                // Make a swap function here.  Makes it clearer what you want to do.
                int temp = v[j].id;

                // What is 100?  How do you know that is enough?
                // These are called magic numbers and lead to code death.
                char temp2[100];
                char temp3[100];

                // Ah, strcpy - 3 things wrong here.
                // 1 - You have the parameters backwards - you are copying temp3 to your struct.
                // 2 - You have no way to know if the destination will fit the source because it copies until it finds a '\0' - very dangerous.
                // 3 - Because your parameters are backwards and temp123 is not initialized, this very well could copy forever.
                // strncpy (also crazy dangerous) at the least should be used and consider using better means like strlcpy() and such.
                strcpy(v[j].prename,temp3);
                strcpy(v[j].name,temp2);
                v[j].id = v[j+1].id;
                v[j+1].id = temp;

                // You kinda forgot to swap the strings, but the program is already dead so no worries.
            }
        }
    }

    // Please enable compiler warnings - you must return a value here.
    return;
}

In seriousness, I'm sure I missed a few other things, but this is good enough for a free internet code review and learning session. :)

Info on strcpy() and strncpy() Why is strncpy insecure?

Info on scanf() safety How to prevent scanf causing a buffer overflow in C?

Info on Variable Length Array safety: Is it safe to use variable-length arrays?

Michael Dorgan
  • 12,453
  • 3
  • 31
  • 61
  • "VLA is now part of newer standards, but are not safe because they cannot fail gracefully on out of memory." This applies also to any function call as a stack may overflow. Nominal use of small VLA pose no real "safety" concerns. OP's use of VLA here is unguarded though and that is a concern. – chux - Reinstate Monica Nov 30 '18 at 19:02
  • More pointing out that VLAs are UB on no memory where malloc/free will at least give a return value you can deal with. Same for `alloca()` – Michael Dorgan Nov 30 '18 at 19:04
  • The real fun begins when we add error checks to get around these with if statements and then become vulnerable to Spectre attacks :) – Michael Dorgan Nov 30 '18 at 19:05