Two dimensional array with char pointer in c

Question

In the following code I am trying for each band to get the number of band members. I've tried a number of things but nothing works. The following looks like it should but doesn't.

If any one could point out what I'm doing wrong it would be greatly appreciated.

numMembers = sizeof(bands[0]) / sizeof(bands[0].members);

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

int main(void) {
    int         i;  
    int         j;  
    int         numBands;
    int         numMembers;
    int         limit = 4;

    struct band {
        char        name[10];
        char        *members[20];
    };  

    const struct band bands[] =
        {   {"Beatles", {"John", "George", "Paul", "Ringo", "Pete", "George"} },
            {"Stones",  {"Mick", "Keith", "Bill", "Charlie", "Brian"} },
            {"Who",     {"Pete", "Roger", "Keith", "John"} },
            {"JHE",     {"Jimmy", "Noel", "Mitch"} }  };  

    numBands   = sizeof(bands) / sizeof(bands[0]);

    for ( i = 0; i < numBands; ++i ) { 
        printf ("%s\n", bands[i].name);
        numMembers = sizeof(bands[0]) / sizeof(bands[0].members);
        for ( j = 0; j < numMembers; ++j )
            printf ("\t%s", bands[i].members[j]);
        printf ("\n");
    }   

    return 0;
}

Design question: why use a fixed array size char name[10] for band names, yet a character pointer for band members? I'd expect something like char *name; char *members[20]; or char name[10]; char members[20][30]; — chux
– chux, Commented Nov 7, 2018 at 23:16

David C. Rankin · Accepted Answer · 2018-11-08 00:54:28Z

You don't have a "two dimensional array". What you have is a simple array of struct band which contains two simple character arrays as its members.

There is actually no need to compute the number of "members" in each bands. All you need to compute is the number of bands, e.g.

    int nbands = sizeof bands / sizeof *bands;

Since you have declared members as an array of pointers and have used an initializer to initialize the first four, the remaining pointers will have all bytes set 0 resulting in each being NULL. You can simply loop for the members with while (bands[i].members[j]), incrementing j each iteration, e.g.

#include <stdio.h>

struct band {
    char    name[10];
    char    *members[20];
};

int main(void) {

    const struct band bands[] = {
        {"Beatles", {"John", "George", "Paul", "Ringo", "Pete", "George"} },
        {"Stones",  {"Mick", "Keith", "Bill", "Charlie", "Brian"} },
        {"Who",     {"Pete", "Roger", "Keith", "John"} },
        {"JHE",     {"Jimmy", "Noel", "Mitch"} } };  

    int nbands = sizeof bands / sizeof *bands;

    for (int i = 0; i < nbands; i++) {
        int j = 0;
        puts (bands[i].name);
        while (j < 20 && bands[i].members[j])
            printf (" %s", bands[i].members[j++]);
        putchar ('\n');
    }
}

(note addition of j < 20, which as pointed out by @chux in the comments, the test of while (bands[i].members[j]) alone could result in Undefined Behavior in the case where there were additional bands added bands after initialization and all 20 pointers of the members struct member were filled)

Example Use/Output

$ ./bin/bandmembers
Beatles
 John George Paul Ringo Pete George
Stones
 Mick Keith Bill Charlie Brian
Who
 Pete Roger Keith John
JHE
 Jimmy Noel Mitch

Additionally, don't use magic numbers in your code. Instead, If you need a constant, #define one (or more), e.g.

#define MAXNM 10
#define MAXMB 20

or use a global enum to do the same thing, e.g.

enum { MAXNM = 10, MAXMB = 20 };

That will allow you to eliminate the magic numbers, e.g.

struct band {
    char    name[MAXNM];
    char    *members[MAXMB];
};

This becomes very important for maintainability as your code length grows.

while (bands[i].members[j]) printf (" %s", bands[i].members[j++]); this is UB when the initialization had 20 band members. Maybe while (j < MAXMB && bands[i].members[j])?
I don't think the MAXNM and MAXNB improve the code . It's obvious what the numbers mean in their original context, and the use of sizeof avoids the possibility of errors due to code duplication
@chux while the check for j < MAXMB is a fine addition, the check as it sits is while (bands[i].members[j] != NULL) which is not UB based on the initialization rules. See § 6.7.9 Initialization (p10) and § 6.7.9 Initialization (p19)
@M.M agreed for this short snippet. The addition was to address future programming where the "code length grows". I try to address all points and what could become bad habits. Sometimes what seems obvious to veterans is useful information for those just learning.
@chux - yes, I see your point, if all 20 band members were later filled, then UB. (but no UB in the code as written)

chux · Accepted Answer · 2018-11-07 22:20:09Z

what I'm doing wrong

Wrong numMembers calculation

numMembers should be the number of elements in the array. (20)

Each bands[i].member has 20 elements given char *members[20]. Several elements are populated with pointers to string literals. Most elements remain 0 (NULL).

// numMembers = sizeof(bands[0]) / sizeof(bands[0].members);
numMembers    = sizeof(bands[0].members) / sizeof(bands[0].members[0]);

Attempting to print NULL as a string

printf ("\t%s", bands[i].members[j]); not valid when bands[i].members[j] is NULL.

Not all bands[i].members[j] are set to a non-NULL value.

numBands = sizeof(bands) / sizeof(bands[0]);

for ( i = 0; i < numBands; ++i ) { 
  printf ("%s\n", bands[i].name);
  numMembers = sizeof(bands[i].members) / sizeof(bands[i].members[0]);

  for ( j = 0; j < numMembers; ++j ) {
    if (bands[i].members[j]) {
      printf ("\t%s", bands[i].members[j]);
    } 
  }

  printf ("\n");
  }
}

Deeper:

const struct band bands[] = { {"Beatles", ... forms bands and sizes bands[] based on the number of initializers.

char *members[20]; is a fixed size even if the initialization did not supply 20 strings. The first elements members[20] are initialized per the list, the remaining elements have a pointer value of 0.

lostbard · Accepted Answer · 2018-11-07 21:59:41Z

For numMembers = sizeof(bands[0]) / sizeof(bands[0].members);

sizeofband(bands[0]) will give the size of one band struct - that is the 10 bytes for name, and then 20 * the size of a pointer.

sizeof(bands[0].members); will give 20*thesizeof a pointer

Not what you want, as sizeof doesn't take into consideration the content of whatever the pointer points to.

It also doesn't take into consideration that some of those members pointers will be NULL so you don't want to count them.

Perhaps better doing:

for ( j = 0; j < 20 &&  bands[i].members[j] != NULL; ++j )
        printf ("\t%s", bands[i].members[j]);

Collectives™ on Stack Overflow

Two dimensional array with char pointer in c

3 Answers 3

5 Comments

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

5 Comments

Comments

Comments

Related