Adding structs to a linked list doesn't seem to work

Question

So I'm doing an exercise in C, learning to manipulate data structures. I am struggling a little with structs/pointers in functions. I've tried to step through, and I can see that I'm creating new structures, but I'm either creating them in the same memory location OR I'm creating a Linked List with multiple of the same struct! I can't work out what is going on.

I've attempted to simplify my code as requested, and this now contains all of the code (I believe the minimum?) required to reproduce my problem.

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

typedef struct aircraft {
    char FlightNumber[9]; // Unique aircraft ID
    struct aircraft * next; // A pointer to the next aircraft in the current queue
} AIRCRAFT;

AIRCRAFT *AirQueue = NULL;
AIRCRAFT *Current = NULL;

void GenerateFlightNumber(AIRCRAFT* node) {

    char FlightNumber[10] = ""; // Stores the flight number for the duration of this function
    int random = 0; // Stores the random number used to generate the flight number prefix and suffix

    // Generates the prefix
    srand((unsigned)time(NULL)); // Uses current time as seed for random generator
    random = rand() % 10; // Generates a random number between 0 and 9
    char prefix[10][5] = { "BA","ELAL","SHT","EXS","EZY","TOM","RYR","MON","UAE","TFL" }; // Array of airline prefixes
    strcpy_s(FlightNumber, sizeof(FlightNumber), prefix[random]); // Copies a prefix to the FlightNumber variable by selecting one using a random index number

    // Generates the suffix
    char suffix[5]; // Stores the flight number suffix
    srand((unsigned)time(NULL)); // Uses current time as seed for random generator
    random = (rand() % 8888) + 1111; // Generate a random number between 1111 and 9999
    _itoa_s(random, suffix, 5, 10); // Converts the randomly generated suffix to a string, and stores it in the suffix variable
    strcat_s(FlightNumber, sizeof(FlightNumber), suffix); // Concatenates the prefix and suffix to the FlightNumber variable

    strcpy_s(node->FlightNumber, sizeof(node->FlightNumber), FlightNumber); // Assign the final flight number to the new aircraft
}

void setUpAircraft(AIRCRAFT * node, bool ground) {
    GenerateFlightNumber(node);
}

AIRCRAFT* StartAirQueue()
{
    printf("\nCreating Air Queue...");
    AIRCRAFT *Temporary = (AIRCRAFT*)malloc(sizeof(AIRCRAFT));
    if (Temporary == NULL)
    {
        printf("\nFailed to allocate memory\n");
        return NULL;
    }
    setUpAircraft(Temporary, false);
    Temporary->next = NULL;

    AirQueue = Current = Temporary;
    return Temporary;
}

AIRCRAFT* AddToAirQueue(bool end)
{
    if (NULL == AirQueue)
    {
        return (StartAirQueue());
    }

    if (end)
        printf("\nAdding node to end of queue...");
    else
        printf("\n Adding node to beginning of queue...");

    AIRCRAFT *Temporary = (AIRCRAFT*)malloc(sizeof(AIRCRAFT));
    if (NULL == Temporary)
    {
        printf("\n Node creation failed \n");
        return NULL;
    }
    setUpAircraft(Temporary, false);
    Temporary->next = NULL;

    if (end)
    {
        Current->next = Temporary;
        Current = Temporary;
    }
    else
    {
        Temporary->next = AirQueue;
        AirQueue = Temporary;
    }
    return Temporary;
}

void print_list(void)
{
    AIRCRAFT *ptr = AirQueue;

    printf("\n -------Printing list Start------- \n");
    while (ptr != NULL)
    {
        printf("\n [%s] \n", ptr->FlightNumber);
        ptr = ptr->next;
    }
    printf("\n -------Printing list End------- \n");

    return;
}

int main(void)
{
    int i = 0, result = 0;
    AIRCRAFT *ptr = NULL;

    print_list();

    for (i = 5; i < 10; i++)
        AddToAirQueue(true);

    print_list();

    for (i = 4; i > 0; i--)
        AddToAirQueue(false);

    print_list();

    getchar();
    return 0;
}

I want to produce a list of aircraft, each with a different flight number. As you can see from the result, each node in the list seems to contain the same flight number.

------Printing list Start-------
------Printing list End------
Creating Air Queue...
Adding node to end of queue...
Adding node to end of queue...
Adding node to end of queue...
Adding node to end of queue...
------Printing list Start-------
[RYR5769]
[RYR5769]
[RYR5769]
[RYR5769]
[RYR5769]
------Printing list End------
Adding node to end of queue...
Adding node to end of queue...
Adding node to end of queue...
Adding node to end of queue...
------Printing list Start-------
[RYR5769]
[RYR5769]
[RYR5769]
[RYR5769]
[RYR5769]
[RYR5769]
[RYR5769]
[RYR5769]
[RYR5769]
[RYR5769]
------Printing list End------

Does anyone have any helpful suggestions as to why my list seems to contain the same node repeatedly?

I've added that - do you need to see the methods that set the properties? It's mostly randomly generated integers. — Link, Apr 14 '18 at 13:38
Please post a [mcve]. This code is neither minimal, complete, nor verifiable. — interjay, Apr 14 '18 at 13:39
C does not support _methods_. And you are expected to post a [mcve] and detailed information. Read [ask]. You should get comfortable with your debugger (the program, not someone voluteering to do your job).. — too honest for this site, Apr 14 '18 at 13:40
We do not need to see the functions that set the properties. Instead, we need to see an altogether different, complete, and much smaller program that demonstrates the same problem you are facing. You may be able to generate such a [mcve] by drastically cutting down a copy of your current program, but you should consider whether building a separate example program from scratch would be more effective. Both approaches have their advantages. — John Bollinger, Apr 14 '18 at 13:45
Thanks for your comments. I've updated the code. I think it's minimal enough - I can't think of anything more to remove and still reproduce the problem. @Olaf - _Apologies_, thank you so much for pointing out my misuse of the word _methods_. It was very useful. — Link, Apr 14 '18 at 16:11
@Link I made an edit to my response but tl;dr it is your random function, the pointer address are most definitely unique. — tijko, Apr 14 '18 at 17:16

tijko · Accepted Answer · 2018-04-15T11:32:02.880

I ported this to run on Archlinux but it shouldn't take much to convert it back.

I added a couple of print statements to show the pointer addresses for each node, you will see they are not the same.

#include<stdio.h>
#include <unistd.h>
#include <time.h>
#include<stdlib.h>
#include<stdbool.h>
#include <string.h>

typedef struct aircraft {
    char FlightNumber[9]; // Unique aircraft ID
    struct aircraft * next; // A pointer to the next aircraft in the current queue
} AIRCRAFT;

AIRCRAFT *AirQueue = NULL;
AIRCRAFT *Current = NULL;

void GenerateFlightNumber(AIRCRAFT* node) {

    char FlightNumber[10] = ""; // Stores the flight number for the duration of this function
    int random = 0; // Stores the random number used to generate the flight number prefix and suffix

    // Generates the prefix
    random = rand() % 10; // Generates a random number between 0 and 9
    printf("Random: %d\n", random);
    char prefix[10][5] = { "BA","ELAL","SHT","EXS","EZY","TOM","RYR","MON","UAE","TFL" }; // Array of airline prefixes
    strncpy(FlightNumber, prefix[random], sizeof(FlightNumber)); // Copies a prefix to the FlightNumber variable by selecting one using a random index number

    // Generates the suffix
    char suffix[5]; // Stores the flight number suffix

    random = (rand() % 8888) + 1111; // Generate a random number between 1111 and 9999
    snprintf(suffix, sizeof(FlightNumber), "%d", random);
    strncat(FlightNumber, suffix, sizeof(FlightNumber)); // Concatenates the prefix and suffix to the FlightNumber variable

    strncpy(node->FlightNumber, FlightNumber, sizeof(node->FlightNumber)); // Assign the final flight number to the new aircraft
}

AIRCRAFT* StartAirQueue()
{
    printf("\nCreating Air Queue...");
    AIRCRAFT *Temporary = (AIRCRAFT*)malloc(sizeof(AIRCRAFT));
    if (Temporary == NULL)
    {
        printf("\nFailed to allocate memory\n");
        return NULL;
    }
    GenerateFlightNumber(Temporary);
    Temporary->next = NULL;
    AirQueue = Current = Temporary;
    return Temporary;
}

AIRCRAFT* AddToAirQueue(bool end)
{
    if (NULL == AirQueue)
    {
        return (StartAirQueue());
    }

    if (end)
        printf("\nAdding node to end of queue...");
    else
        printf("\n Adding node to beginning of queue...");

    AIRCRAFT *Temporary = (AIRCRAFT*)malloc(sizeof(AIRCRAFT));
    if (NULL == Temporary)
    {
        printf("\n Node creation failed \n");
        return NULL;
    }
    GenerateFlightNumber(Temporary); 
    Temporary->next = NULL;

    if (end)
    {
        Current->next = Temporary;
        Current = Temporary;
    }
    else
    {
        Temporary->next = AirQueue;
        AirQueue = Temporary;
    }
    return Temporary;
}

void print_list(void)
{
    AIRCRAFT *ptr = AirQueue;

    printf("\n -------Printing list Start------- \n");
    while (ptr != NULL)
    {
        printf("\n [%s] [%p]\n", ptr->FlightNumber, ptr);
        ptr = ptr->next;
    }
    printf("\n -------Printing list End------- \n");

    return;
}

int main(void)
{
    int i = 0;

    print_list();
    srand((unsigned)time(NULL));
    for (i = 5; i < 10; i++) {
        AddToAirQueue(true);
        sleep(1);
    }
    print_list();

    for (i = 4; i > 0; i--) {
        sleep(1);
        AddToAirQueue(false);
    }

    print_list();

    getchar();
    return 0;
}

As I have said from when you first posted, the issue has been with your "random" function.

This line specifically:

srand((unsigned)time(NULL)); // Uses current time as seed for random generator

Just move it into your main because you only have to call it once.

You can read more on seeding from previous questions.

By seeding with seconds (since the epoch) with time(NULL), your calls were happening in such tight succession that the seed is the same each time giving you the same number repeatedly.

Thanks for your answer. But that's not quite what I'm looking for. I've updated my code - it reproduces the problem, but without any of that conditional code. The problem I have is that my list seems to contain multiple of the same node - or multiple nodes with the same flight number, which I am trying to randomly generate. — Link, Apr 14 '18 at 16:15
I printed the addresses of each pointer in the list, and each one is the same. I'm not using that random function anymore. — Link, Apr 14 '18 at 16:23
It is, I've tried to minimise it to reproduce the actual problem without any distraction. — Link, Apr 14 '18 at 16:25
Yeah, I've been made aware of that :p hence the change. I actually pretty much rewrote the whole program, just copied a few bits of the old one. — Link, Apr 14 '18 at 16:28
Thank you. It was the seed causing my problem. Everything else works as I expect it to, phew! RE: best practises, etc. I'm very new to C, and quite new to programming generally, as I'm sure you guessed. Forgive me if anything really pains you to read :p but I'm trying to learn by doing, and this was a stumbling block I couldn't figure out. Thank you for taking the time to help me out, I'm going to go away and read about seeding now. — Link, Apr 14 '18 at 17:49
@Link way to go! They way the seeding works is a little unintuitive. I learn some myself. — tijko, Apr 14 '18 at 17:52

Adding structs to a linked list doesn't seem to work

1 Answers1