C - Creating a dynamic array of structs, struct members printing wrong values?

Question

I am trying to create a dynamic array of players which I can add to during run time - however if I create 3 players with x-coords: 4,7 and 15, then try to print these values the output is: 0, 33, 20762704.

I am new to C and pointers and am struggling to work out where it is going wrong.

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

// contains data of a player
struct player {
    int posX;
    int posY;
    int gold;
};

// struct for creating a list of players of dynamic size
struct playerList {
    struct player p;    
    struct playerList *next;
};


// add a new player to the list with given coords
struct playerList *make(int x, int y) {
    struct playerList *new_player;
    new_player = (struct playerList *)malloc(sizeof(struct playerList));
    new_player->p.posX = x;
    new_player->p.posY = y;
    new_player->p.gold = 0;
    new_player->next = NULL;
    return new_player;
}

// add a player to the list
void addPlayer(struct playerList *list, int x, int y) {
    if(list->next) {
        addPlayer(list->next,x,y);
    }
    else {
        list->next = make(x,y);
}}


int main() {
    struct playerList *players = (struct playerList *)malloc(sizeof(struct playerList));

    addPlayer(players, 4,3);
    addPlayer(players, 7,7);
    addPlayer(players,15,1);

    printf("%d\n",players[0].p.posX);
    printf("%d\n",players[1].p.posX);
    printf("%d\n",players[2].p.posX);

    return 0;

}

Answer 1

In the list, you have a node that you are going to save some data on it, and it points to the next node too. So, you could define list structure to maintain the head of your list, and probably some other required information such length of the list or garbage handling or ...

For initialization you should set the length with zero and head pointer of list to NULL, these steps show the empty status of the list.

When you want to add to the list, you could add at the end of it, or at the head of it. In your program, you choose the second insertion policy, at the end. So, to add, you should traverse the list (all nodes), to find the last node, to add new node after that one. You should be aware of adding the new node when the list is empty, in this case you should update the head of your list. For printing, there is a similar way, you should traverse the list and print the node information of that, until you reach the null pointer at the end of list.

After any allocation you should check the allocation success, if the pointer is not null, it was successful.

Another point, when you can handle adding the new node with using a simple loop, why you should use the recursive function? In this cases, it is better to use the loop.

The last point, dynamic allocation memory used commonly when the number of the list is specified in the run time, for example. It is a good point, to less memory allocation if you don't have to use. For instance, in the main you could define the list variable as a static variable, and send the address of that to the functions.

I tested the program, and its output was okay.

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

// contains data of a player
struct player {
    int posX;
    int posY;
    int gold;
};

// struct for creating a list of players of dynamic size
struct playerNode {
    struct player p;
    struct playerNode *next;
};

struct playerList {

    struct playerNode *head;
    int len;
    // Add other required variables here
};

// add a new player to the list with given coords
struct playerNode *make(int x, int y) {
    struct playerNode *new_player;
    // you need to check memory allocation success
    new_player = malloc(sizeof(struct playerNode));
    new_player->p.posX = x;
    new_player->p.posY = y;
    new_player->p.gold = 0;
    new_player->next = NULL;
    return new_player;
    }
// add a player to the list
void addPlayer(struct playerList *list, int x, int y) {
    struct playerNode *player = list->head;
    if(!player)
        // you need to check memory allocation success
        list->head = make(x, y);
    else
    {
        while (player->next) {
                player = player->next;
        }
        // you need to check memory allocation success
        player->next = make(x, y);
    }
    list->len++;
}

void showPlayers(struct playerList *list) {
    struct playerNode *player = list->head;
    while (player) {
        printf("%d\n", player->p.posX);
        printf("%d\n", player->p.posY);
        printf("%d\n", player->p.gold);
        printf("--------------------\n");
        player = player->next;
    }
}

int main() {
    struct playerList players;
    players.len = 0;
    players.head = NULL;

    addPlayer(&players, 4, 3);
    addPlayer(&players, 7, 7);
    addPlayer(&players, 15, 1);

    showPlayers(&players);
    return 0;

}

Answer 2

In order to add the first player to the list, you must pass a pointer-to-pointer-to-playerList to addPerson because the first node address will become the list address. Otherwise, you must return type *playerList and assign the return to your list variable back in the calling function. It is just as easy to pass the playerList ** parameter to your function and return a pointer to indicate success/failure as well as for convenience. e.g.:

/* add a player to the list */
playerList addPlayer (struct playerList **list, int x, int y) {

    struct playerList *node = make (x, y);
    if (!node) {  /* validate new player created */
        fprintf (stderr, "error: make player failed for (%d,%d).\n", x, y);
        return NULL;
    }

    if (!*list)  /* if first node, set list address to node & return */
        return *list = node;

    struct playerList *iter = *list;   /* list pointer to iterate to end */

    /* insert all other nodes at end */
    for (; iter->next; iter = iter->next) {}

    iter->next = node;   /* add new player at end, return original *list */

    return *list;
}

Then in main

addPlayer(&players, 4,3);
...

(note: the addPlayer is no longer recursive. As your list size grows, the additional resources needed for recursive calls can become significant, further, there is no need for a recursive call as the procedural iteration to the end of list to add a new player is straight forward.)

Look over the change and let me know if you have any additional questions. (note: I have not checked the remainder of your code for further errors)