removing user chosen nodes from a linked list, replacing them with nodes from another list in c

Question

I'm trying to write a poker program. Pretty much everything works right now, the one issue is where the program asks the user which cards they want to keep, and which they want to discard. If it worked as intended, the user would enter which cards they want to keep. The cards that weren't selected would then be removed, and replaced with cards from the deck. The player's hand and the deck are two separate linked lists.

This part of the program behaves kind of strangely. Sometimes it works fine. Other times, it discards cards that were meant to be kept, or keeps card that were meant to be discarded. It sometimes also changes the suit of some of the cards (or maybe it's duplicating certain cards, I'm not sure).

This is the function that creates the deck:

card *
createCard(int n)
{
    int i = 0;
    card *head = (card *) malloc(sizeof(card));
    card *tmp = NULL;
    card *p = NULL;

    p = head;

    for (i = 0; i < n - 1; i++) {
        tmp = (card *) malloc(sizeof(card));
        p->next = tmp;
        p = p->next;
        p->next = NULL;
    }

    tmp = head;
    for (i = 1; i <= 13; i++) {

        for (int j = 3; j <= 6; j++) {
            tmp->face = i;
            tmp->suit = j;
            tmp = tmp->next;
        }

    }

    return (head);
}

This is the function that creates the player's hand(by creating a new linked list from the first five nodes of the deck list):

void
createHand(card ** deck, card ** hand)
{
    card *tmp = NULL;
    card *card = *deck;
    int i;

//while (card != NULL)
    for (i = 0; i < 5; i++) {
        (*deck) = (*deck)->next;

        tmp = card->next;
        card->next = *hand;
        *hand = card;
        card = tmp;

    }
    (*hand)->next->next->next->next->next = NULL;
    return;

}

This is the section of code that's not working(note: playerHand is the player's hand, first is the deck):

i = 1;

// this array keeps track of which cards the user wants to keep
int a[] = { 0, 0, 0, 0, 0 };

while (i <= 5) {
    printf("Pick cards (between 1-5) to hold (-1 to stop): ");
    scanf("%d", &keep);

    // breaks from loop if the user enters -1
    if (keep == -1)
        break;

    if (keep == 0 || keep > 5 || keep <= -2) {
        printf("Invalid index. Pick cards (between 1-5) to hold (-1 to stop): ");
        scanf("%d", &keep);
        if (keep == -1)
            break;
    }
    if (keep == -1) {
        break;
    }
    if (keep != -1) {
        // when player wants to keep a card, the corresponding index of that
        // card is set to one in the array
        a[keep - 1] = 1;
    }
    i++;
}

card *tmp;

tmp = first;
card *tmp2;

tmp2 = playerHand;
for (i = 0; i < 5; i++) {
    // if the corresponding index in the array is 0, that card is replaced
    if (a[i] == 0) {
        tmp2->face = tmp->face;
        tmp2->suit = tmp->suit;
    }
    first = first->next;
    free(tmp);
    tmp = first;
    tmp2 = tmp2->next;
}

The cards don't change when this section of code is removed, so the error must be here somewhere, I'm just not sure where.

This is what the output would look like while the player is choosing which cards to keep. In this case, the player is choosing to keep the first and third cards and discarding the other three:

Pick cards (between 1-5) to hold (-1 to stop): 1

Pick cards (between 1-5) to hold (-1 to stop): 3

Pick cards (between 1-5) to hold (-1 to stop): -1

Have you tried running your code line by line in a debugger while monitoring the values of all variables, in order to determine at which point your program stops behaving as intended? If you did not try this, then you may want to read this: [What is a debugger and how can it help me diagnose problems?](https://stackoverflow.com/q/25385173/12149471) You may also want to read this: [How to debug small programs?](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). — Andreas Wenzel, Dec 16 '21 at 22:05
Note that when debugging a program which generates random values, it is often helpful to seed the random number generator with a fixed value, instead of a different value every time the program is run. That way, you can reproduce specific errors more easily, as the behavior of the program will be identical, whenever you restart your program in the debugger. — Andreas Wenzel, Dec 16 '21 at 22:18
If possible, please provide a [mre] of the problem (which includes a function `main` and all `#include` directives). For example, maybe you can you provide a simple hard-coded linked list for the player's hand and the deck, and demonstrate the problem on that. — Andreas Wenzel, Dec 16 '21 at 22:33
You have UB (undefined behavior). In the 3rd code block, in the `for` loop, you do: `tmp2->face = tmp->face;`. But, `tmp` is _unitialized_. It has a "random" value, so it could point to anything. Usually, this causes a segfault. But, with UB, if `tmp` points to memory that exists, you can just get random results, based on _whatever_ just "happens" to be there. You would want `tmp` to point to a fresh card pulled/dequeued from the deck. So: under the `if`, you might do: `tmp = get_card_from_deck()`. But, this would "leak" the card that `tmp` points to ... — Craig Estey, Dec 17 '21 at 02:25
... Better to dequeue/free `tmp2` and link in `tmp` where `tmp2` is in the list of the player's hand. I'd write functions that perform low level tasks (e.g): `void list_append(list *lst,card *crd);` and `void list_remove(list *lst,card *crd);` and `void list_discard_and replace(list *lst,card *old,card *new);` You probably want a few lists: `list discard_list, deck_list, player_hands[NPLAYERS];` And, move cards to/from these lists — Craig Estey, Dec 17 '21 at 02:30

arutar · Answer 1 · 2021-12-17T23:22:20.173

Perhaps the code should look like this? How does the player choose the replacement cards?

Can you show what the code for saving cards for replacement looks like?

card *tmp;

tmp = first;
card *tmp2;

tmp2 = playerHand;
for (i = 0; i < 5; i++) {
    if (a[i] == 0) {//if the corresponding index in the array is 0, that card is replaced
        tmp2->face = tmp->face;
        tmp2->suit = tmp->suit;

        first = first->next;
        free(tmp);
        tmp = first;
    }
    tmp2 = tmp2->next;
}

After the user chooses the cards to replace, we take the first n cards from the deck.

The mistake in your code is that the top five cards of the deck are always discarded. If the card in the hand was marked for discarding, then not the next card from the deck was taken, but a card with the same index.

card *tmp;

tmp = first;
card *tmp2;

tmp2 = playerHand;
for (i = 0; i < 5; i++) {
    // if the corresponding index in the array is 0, 
    // the card will be replaced by a card with the same index from the deck
    if (a[i] == 0) {
        tmp2->face = tmp->face;
        tmp2->suit = tmp->suit;
    }
    first = first->next; //changes the top of the deck to the next
    free(tmp);    //always removes a card from the deck
                  //so the first 5 cards will be removed
    tmp = first;  //all these three lines must be moved to the condition if

    tmp2 = tmp2->next;
}

and in the while loop, you need to change the code, because if the user enters the wrong index twice in a row, the program will crash

    if (keep == 0 || keep > 5 || keep <= -2) {
        printf("Invalid index. ");
        continue;
    }

removing user chosen nodes from a linked list, replacing them with nodes from another list in c

1 Answers1