C: accessing members of struct in a function

Question

I'm trying to use the struct member 'size' in my function print_shoe, but my for loop doesn't run. However, if I replace 'c->size' with an int in the for loop, it runs just fine

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define DECK_SIZE 52
#define NUM_FACES 13
#define NUM_SUITS 4
#define LENGTH_FACES 6
#define LENGTH_SUITS 9

typedef struct cards {
  char suits[NUM_SUITS][LENGTH_SUITS];
  char faces[NUM_FACES][NUM_FACES];
  int suit, face, card, value, size;
  int *values[NUM_FACES];
} cards;

char buf[101];
void print_shoe();
void init_decks();
int rand_int();
void shuffle();

int main(void) {

  srand( time(NULL) );

  int decks_input = 0;    
  int numberOfDecks = 1;

  do {
    printf("\nEnter number of decks to be used in the game (1-8):\n\n");
    if (fgets(buf, sizeof(buf), stdin) != NULL)
      if (sscanf (buf, "%d", &decks_input))
        numberOfDecks = decks_input;
     } while (numberOfDecks < 1 || numberOfDecks > 8);

  cards *shoe = malloc(sizeof(cards) * numberOfDecks * DECK_SIZE);
  shoe->size = numberOfDecks * DECK_SIZE;

  shuffle(shoe);
  print_shoe(shoe);

  free(shoe);

  return 0;
}

void print_shoe(cards *c) {
  int i;
  for (i = 0; i < c->size; i++) {
    printf("card #%d = %s of %s\n", i+1, c->faces[c[i].face], c->suits[c[i].suit]);
  }
}

void init_decks(cards *c) {
  int i;
  for (i = 0; i < c->size; i++) {
    c[i].card = i;
    c[i].suit = c[i].card % NUM_SUITS;
    c[i].face = c[i].card % NUM_FACES;
  }  
}

void shuffle(cards *c) {
  init_decks(c);

  int i, j;
  cards tmp;
  for (i = c->size - 1; i > 0 ; i--) {
    j = rand_int(i + 1);
    tmp = c[j];
    c[j] = c[i];
    c[i] = tmp;
  }
}

int rand_int(int n) {
  int limit = RAND_MAX - RAND_MAX % n;
  int rnd;

  do {
    rnd = rand();
     } while (rnd >= limit);
  return rnd % n;
}

Edit: Question has been updated extensively in response to comments that it needed more clarification

Your code does not compile. 1. put print_shoe before main 2. (cards *)malloc(...). After I change these, it runs good on my machine. — gongzhitaao
– gongzhitaao, Commented May 13, 2013 at 23:06
@gongzhitaao: The cast is unnecessary and a bad idea. Just be sure yuou have #include <stdlib.h>, and the conversion from void* to cards* will be done implicitly. — Keith Thompson
– Keith Thompson, Commented May 13, 2013 at 23:22
You haven't defined buf or DECK_SIZE, and you're missing the required #include directives for <stdio.h> and <stdlib.h>. And you say your function "can't access" the struct members. What does that mean? What happens when you try? Do you get a compile-time error messages? If so, show it to us. Show us a complete sample program that exhibits the problem, and tell us what the problem is. — Keith Thompson
– Keith Thompson, Commented May 13, 2013 at 23:30
printf("card #%d = %s of %s\n", i+1, c->faces[c[i].face], c->suits[c[i].suit]); you refer to the part that has not been initialized. — BLUEPIXY
– BLUEPIXY, Commented May 13, 2013 at 23:59
A debugger would have shown this pretty readily: You initialized the size of the first cards in the array pointed to by shoe, and the other cards have uninitialized values for size. When you shuffle, one of the uninitialized cards becomes the first entry in the shoe, and you are using an uninitialized variable. The size does not belong in the cards structure. — Raymond Chen
– Raymond Chen, Commented May 14, 2013 at 0:14

Jonathan Leffler · Accepted Answer · 2013-05-14 01:52:27Z

1

In the revised code, you have:

cards *shoe = malloc(sizeof(cards) * numberOfDecks * DECK_SIZE);
shoe->size = numberOfDecks * DECK_SIZE;

// You probably need init_decks(shoe); here!!!

shuffle(shoe);
print_shoe(shoe);

Your code in print_shoe() is simply printing, but you've not initialized the data from malloc() other than the size, so you're printing garbage. The data returned by malloc() is uninitialized and must be initialized before it is read. The question changed while I was typing; you still haven't called init_decks(shoe); as you need to.

This is not why it isn't working — I'm not sure what the problem is now — but it is almost worth commenting on. You have:

void shuffle(cards *c) {
  init_decks(c);

  int i, j;
  cards tmp;
  for (i = c->size - 1; i > 0 ; i--) {
    j = rand_int(i + 1);
    tmp = c[j];
    c[j] = c[i];
    c[i] = tmp;
  }
}

If you're going to use the C99 technique of declaring variables with minimal scope, then you should be writing:

void shuffle(cards *c) {
  init_decks(c);
  for (int i = c->size - 1; i > 0; i--) {
    int j = rand_int(i + 1);
    cards tmp = c[j];
    c[j] = c[i];
    c[i] = tmp;
  }
}

(I wouldn't have missed the call to init_decks() had it been written like that.)

As noted in a comment, your cards structure is rather top-heavy. You allocate (for each card) enough space to store the ranks and suits that it could possibly have. This is really not necessary.

This code separates a Deck from a Card. It uses a flexible array member in the Deck structure to hold the cards, which is convenient. You may prefer to use a regular pointer there, in which case you'll need a pair of memory allocations and a function deck_free() to release the memory allocated by deck_alloc().

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

#define NUM_FACES 13
#define NUM_SUITS 4
#define DECK_SIZE (NUM_FACES * NUM_SUITS)
#define LENGTH_FACES 6
#define LENGTH_SUITS 9

static const char suits[NUM_SUITS][LENGTH_SUITS] =
{
    "Clubs",
    "Diamonds",
    "Hearts",
    "Spades"
};
static const char faces[NUM_FACES][NUM_FACES] =
{
    "Ace",
    "Deuce",
    "Three",
    "Four",
    "Five",
    "Six",
    "Seven",
    "Eight",
    "Nine",
    "Ten",
    "Jack",
    "Queen",
    "King",
};

typedef struct Card
{
  int suit;
  int face;
  int card;
} Card;

typedef struct Deck
{
  int size;
  Card cards[];     // Flexible array member
} Deck;

void print_shoe(const Deck *d);
void init_decks(Deck *d);
int rand_int(int n);
void shuffle(Deck *d);
static Deck *deck_alloc(int numberOfDecks);

int main(void)
{
    srand( time(NULL) );
    int numberOfDecks = 1;

    do
    {
        char buf[101];
        printf("\nEnter number of decks to be used in the game (1-8):\n\n");
        if (fgets(buf, sizeof(buf), stdin) != NULL)
        {
            int decks_input;    
            if (sscanf (buf, "%d", &decks_input))
                numberOfDecks = decks_input;
        }
    } while (numberOfDecks < 1 || numberOfDecks > 8);

    Deck *shoe = deck_alloc(numberOfDecks);
    shuffle(shoe);
    print_shoe(shoe);
    free(shoe);

    return 0;
}

static Deck *deck_alloc(int numberOfDecks)
{
    Deck *shoe  = malloc(sizeof(Deck) + (sizeof(Card) * numberOfDecks * DECK_SIZE));
    if (shoe == 0)
    {
        fprintf(stderr, "out of memory\n");
        exit(1);
    }
    shoe->size = numberOfDecks * DECK_SIZE;
    return shoe;
}

void print_shoe(const Deck *d)
{
    for (int i = 0; i < d->size; i++)
        printf("card #%d = %s of %s\n", i+1, faces[d->cards[i].face], suits[d->cards[i].suit]);
}

void init_decks(Deck *d)
{
    for (int i = 0; i < d->size; i++)
    {
        d->cards[i].card = i;
        d->cards[i].suit = d->cards[i].card % NUM_SUITS;
        d->cards[i].face = d->cards[i].card % NUM_FACES;
    }  
}

void shuffle(Deck *d)
{
    init_decks(d);
    for (int i = d->size - 1; i > 0 ; i--)
    {
        int j = rand_int(i + 1);
        Card tmp = d->cards[j];
        d->cards[j] = d->cards[i];
        d->cards[i] = tmp;
    }
}

int rand_int(int n)
{
    int limit = RAND_MAX - RAND_MAX % n;
    int rnd;

    do
    {
        rnd = rand();
    } while (rnd >= limit);
    return rnd % n;
}

Sample output:

$ ./cards

Enter number of decks to be used in the game (1-8):

1
card #1 = Eight of Clubs
card #2 = Jack of Clubs
card #3 = Deuce of Diamonds
card #4 = Jack of Hearts
card #5 = Queen of Clubs
card #6 = Four of Hearts
card #7 = Six of Spades
card #8 = King of Hearts
card #9 = Five of Spades
card #10 = King of Clubs
card #11 = Deuce of Clubs
card #12 = King of Spades
card #13 = Four of Spades
card #14 = Nine of Diamonds
card #15 = Five of Hearts
card #16 = Deuce of Spades
card #17 = Ten of Clubs
card #18 = Five of Diamonds
card #19 = Ten of Spades
card #20 = Three of Spades
card #21 = Nine of Hearts
card #22 = Six of Clubs
card #23 = Ace of Clubs
card #24 = Three of Clubs
card #25 = Queen of Hearts
card #26 = Jack of Diamonds
card #27 = Nine of Clubs
card #28 = Four of Clubs
card #29 = Seven of Spades
card #30 = Ace of Diamonds
card #31 = Six of Diamonds
card #32 = Three of Hearts
card #33 = Queen of Diamonds
card #34 = Ten of Hearts
card #35 = Ten of Diamonds
card #36 = Seven of Diamonds
card #37 = Seven of Clubs
card #38 = Deuce of Hearts
card #39 = Ace of Hearts
card #40 = Jack of Spades
card #41 = Eight of Diamonds
card #42 = Eight of Spades
card #43 = Ace of Spades
card #44 = Three of Diamonds
card #45 = Queen of Spades
card #46 = Five of Clubs
card #47 = Four of Diamonds
card #48 = King of Diamonds
card #49 = Nine of Spades
card #50 = Eight of Hearts
card #51 = Six of Hearts
card #52 = Seven of Hearts
$

edited May 14, 2013 at 1:52

answered May 14, 2013 at 0:09

Jonathan Leffler

759k145 gold badges961 silver badges1.3k bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

kensing Over a year ago

init_decks(shoe) gets called inside shuffle (even though that might be bad practice). I apologize for the editing, tried doing it asap - I really don't want to waste anyone's time

kensing Over a year ago

I'm not sure what you mean by initializing data returned by malloc()

Jonathan Leffler Over a year ago

I missed the call to init_decks() in shuffle(), and when I started to answer, there wasn't a call to shuffle() either. Initializing the data means that before you try reading anything from the space returned by malloc(), you write something to it. There's no guarantee that you'll get zeros or any other value in the allocated space; it will generally be a quasi-random mishmash of bytes (but it would not be a good source of entropy for random processes).

kensing Over a year ago

can you provide an example?

Jonathan Leffler Over a year ago

Your init_decks() does some initialization — it writes to some elements of the cards structure (and fortunately, NUM_DECKS == NUM_FACES * NUM_SUITS). However, your cards structure is a mixture of several descriptions. You've got enough storage space for a full deck of card names in each entry, but you only have one face and one card and one suit. You need to clean up the structure. You also need to set everything. The standard way of initializing allocated memory is with memset() (usually specifying 0 as the byte value) or by using calloc() instead of malloc().

|

A.E. Drew · Accepted Answer · 2013-05-14 02:34:09Z

Addressing the question directly and ignoring the wisdom of this approach, your problem is as follows(as also mentioned by Raymond Chen).

  cards *shoe = malloc(sizeof(cards) * numberOfDecks * DECK_SIZE);

The line above makes shoe point to enough memory to store (numberOfDecks * DECK_SIZE) struct cards. The structs, and thus there members, are all unitialized at this point meaning shoe[i].size could be any sequence of bits.

shoe->size = numberOfDecks * DECK_SIZE;

This line looks at the only the first struct cards and sets its size to (numberOfDecks * DECK_SIZE). The rest of the struct cards's size members remain unititialized.

In shuffle, your call to init_decks initializes card, suit, and face but not size. When you later shuffle the cards, a card with an unititialized size member has a good chance of becoming first.

Thus, under your current approach, the following should get what you want if you add this line to init_decks.

void init_decks(cards *c) {
      int i;
      int size = c->size;
      for (i = 0; i < c->size; i++) {
            c[i].size = size;
            c[i].card = i;
            c[i].suit = c[i].card % NUM_SUITS;
            c[i].face = c[i].card % NUM_FACES;
       }  
}

Mahesh · Accepted Answer · 2013-05-13 22:37:21Z

0

You have declared a pointer but haven't initialized it a valid memory location to point to.

 ex *ex_p = malloc(sizeof(ex));
 ex_p->size = 10;

answered May 13, 2013 at 22:37

Mahesh

34.7k23 gold badges93 silver badges116 bronze badges

2 Comments

kensing Over a year ago

I did do that in my code, unfortunately.. Can you spot anything else? It's as if loop doesn't even run.. But doesn't give me any errors either. If I simply swap 'ptr->size' in my for loop with the value '10', it works out fine

Mahesh Over a year ago

If you have just once instance, there is no point of iterating or using a for loop. Can you please post your complete actual code ?

Collectives™ on Stack Overflow

C: accessing members of struct in a function

3 Answers 3

6 Comments

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

6 Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related