0

I am new to c programming and I am stuck with this one its a typedef struct and what I would like to do is that I want to create an array from the double pointer from this structure

typedef struct
{
 char* firstname;
 float   price;
}Name,*pName,**ppName;

typedef struct
{
 ppName Names;
 unsigned int numPerson;
}Book;

And my main which always give me segmentation fault dont mind the loop it is looping until the use says to quit.

 int main(void)
{

 Book D;
 setUpCollection(&D);

  while(..)
 {


  scanf(...);
  switch(...)
  {
  case 1:
   if(!AddNewPerson(&D))
    return 1;
   break;
  case 2:
  ....
  case 3:
   ....
  default:
   printf("Please enter a valid choice");
  }
 }
 return 0;
}

void setUpCollection(Book* data){
 Name name;
 pName pname;


 pname= malloc(MAX_PERSON* sizeof(pName));

 pname= &name;

 data->Names= &pname;
 data->numPerson= 0;

}

BOOL AddNewPerson(Book* data){
 char *title = malloc(sizeof(char));
 int len;
 Name name;
 pName pname;


 scanf(...);
 len = strlen(firstname);

 name.firstname = malloc(len * sizeof(char*));
 name.firstname  = firstname;

 pname= malloc(1);
 pname= &name;

 data->DVDs[data->numPerson++] = pname;

  printf("%0.2f", data->Names[(data->numPerson)-1]->price); 

 return TRUE;
}

My main problem is that I cant print all the added names and also getting segmentation fault.

game on
  • 347
  • 1
  • 7
  • 22
  • sorry i was wirting the code not cpying and pasting it... fixed... – game on Jan 09 '14 at 06:03
  • `pname= malloc(1);` ?? `data->DVDs[data->numPerson++] = pname;` ??? – Sourav Ghosh Jan 09 '14 at 06:06
  • You should go ahead and copy whatever code you've got. What you have up here now isn't helpful. – Keeler Jan 09 '14 at 06:07
  • I will im really sorry i am really bad at pointers which i hate the most – game on Jan 09 '14 at 06:11
  • Using typedefs to hide pointers makes matters worse, in my opinion. I would not use the `pName` or `ppName` types unless someone forced me to do so, and I'd write the code without them, and place them in when necessary before submitting the code for scrutiny. They are not IMNSHO a good idea. It also helps if you provide compilable code that reproduces your problem (see SSCCE – [Short, Self-Contained, Correct Example](http://sscce.org/)). Comments about 'never mind the loop' mean 'worry about the loop' because, depressingly often, the trouble is in the bit that you're not supposed to mind. – Jonathan Leffler Jan 09 '14 at 06:59
  • See [typedef pointers — a good idea?](http://stackoverflow.com/questions/750178/typedef-pointers-a-good-idea/) for a discussion of why not to use typedefs of pointers. – Jonathan Leffler Jan 09 '14 at 07:00
  • @JonathanLeffler sorry i really need the typedef pointer and double pointer it is a challenge question my question is how can i allocate memory to Names? just using malloc. it is possible? – game on Jan 09 '14 at 07:08

1 Answers1

2

There are quite a few errors in your program but let me mention a few:


Doesn't this seem odd to you:

pname= malloc(MAX_PERSON* sizeof(pName));
pname= &name;

you are creating a memory leak by first letting pname point to the array of pName then assigning to &name.


What is this:

char *title = malloc(sizeof(char)); // ?

here you allocate too less space

name.firstname = malloc(len * sizeof(char*));

it should be

name.firstname = malloc(len * sizeof(char) + 1);

or more readable:

name.firstname = malloc(len+1);

this makes no sense again:

pname= malloc(1);
pname= &name;

again you created a memory leak by first letting pname point to a heap block of 1 byte then assigning it to a local variable which you include in data - the local variable is freed up once you leave AddNewPerson() so data will point to garbage.


Instead do something like this (I am no fan of having typedefs for pointers), also try avoiding naming types the same way you name variables for clarity:

typedef struct
{
  char *firstname;
  float price;
} Name;

typedef struct
{
  Name** names;
  unsigned int numPerson;
} Book;

Now allocate the initial size of your array, the whole point of having it on the heap is that the array can grow if more records are added than MAX_PERSONS so you need to keep track of the number of used records in the array as well as the number of records allocated

int allocated = MAX_PERSONS;
Book D;
D.names = malloc( allocated * sizeof(Name*) );
D.numPerson = 0; 

then loop over user input and add records keeping track of how many records have been read. Since names is an array of pointers, you need to allocate a Name struct each time you add an entry

e.g.

D.names[i] = malloc( sizeof(Name) );
D.names[i]->firstname = strdup(userInputName);
D.names[i]->price = userInputPrice;

then at each iteration check if there is allocated memory left

++i;
if ( i == allocated )
{
  // if yes you need to get more memory, use realloc for that
  // get e.g. 10 more records
  Name* tmp = realloc( D.names, (allocated + 10)*sizeof(Name) ); 
  if ( tmp != NULL )
  {
    D.names = tmp;
    allocated += 10;
  }
  else 
  { .. some error msg .. }
}
AndersK
  • 35,813
  • 6
  • 60
  • 86
  • what do you think the best approach just in the addnewperson function?. I am really bad at pointers. sorry – game on Jan 09 '14 at 06:39
  • sorry but i do really need the double pointer and pointer within the typededf thats the part i am stuck with.. BTW thx for the answer. – game on Jan 09 '14 at 06:43
  • ok if you replace the firstname[] with a pointer you just need to allocate that as well. you can e.g. use strdup to allocate a copy of the user input - updated answer – AndersK Jan 09 '14 at 06:44
  • are you able to do chat?? need to ask about it – game on Jan 09 '14 at 06:55
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/44856/discussion-between-claptrap-and-game-on) – AndersK Jan 09 '14 at 07:56