C Segmentation Fault - linked lists

Question

I just started programming in C a few weeks ago and I am getting a segmentation fault in my program.

I believe it is because of those lines:

for (int i =0; i < HOW_MANY; i++) 
         {
        people = people -> next;
        if (people -> next == NULL) return people;
     } //for
      }//else

Here is my C program with comments based on my psedocode

struct person *insert_end (struct person *people, char *name, int age) {
//create a new space for the new person
  struct person *pointer = malloc(sizeof(struct person));
   // check it succeeded
    if(pointer == NULL)
    { 
     printf("The program could not allocate memory ");
      exit(-1);
    }
     // set the data for the new person
      strcpy(pointer -> name, name);
      pointer -> age = age;
      pointer -> next = people;
     // if the current list is empty
      if (people == NULL)
      {
        // set the new person's "next" link to point to the current list"
    pointer -> next = people;
    // return a pointer to the new person
    return pointer;
      }
      else  
      {
     // we need a loop to find the last item in the list 
    // (the one which as a "next" link of NULL)
         for (int i =0; i < HOW_MANY; i++) 
         {
       // set the "next link of this item to point
           // to the new person, so that the person
           // becomes the last item in the list
           // (the next person should have a "next" link of NULL)
        people = people -> next;
        if (people -> next == NULL)
        return people;
     } //for
      }//else
       // return the start of the list
       return pointer;    
}

Also, let me know in case you need my full C code for the program, since this is only a method

Thank you,

Sarah :)

Answer 1

This is totally a question for Code Review, but since you did ask a question regarding how to implement an algorithm, at least I think it is suitable for Stack Overflow.

For starters, this is just a syntax thing, but I think you should replace this:

(*pointer).age = age;

with this

pointer -> age = age;

This notation is a lot cleaner in my opinion, and was incorporated into C for the purpose of accessing elements from a struct pointer. Other than that the code you have so far is missing some closing braces. Here's a version of your code with proper syntax, and other important stuff (such as what @H2CO3 mentioned):

struct person *insert_end (struct person *people, char *name, int age) {

//create a new space for the new person
struct person *pointer = malloc(sizeof(struct person));

// check it succeeded
if(pointer == NULL)
{ 
    printf("The program could not allocate memory ");
    exit(-1);
}

// set the data for the new person
strcpy(pointer -> name, name);
pointer -> age = age;
pointer -> next = people;

// if the current list is empty
if (person == 0)
{
    // set the new person's "next" link to point to the current list"
    pointer -> next = people;

    // return a pointer to the new person
    return pointer;
}

else
{
    // we need a loop to find the last item in the list 
    // (the one which as a "next" link of NULL)
    for (int i =0; i < HOW_MANY; i++) 
    {
        // set the "next link of this item to point
        // to the new person, so that the person
        // becomes the last item in the list
        // (the next person should have a "next" link of NULL)
        lastItem -> next = people;
    }

    // return the start of the list
    return pointer;    
}

}

Lastly, since I don't have a full copy of your entire code, nor do I want it, I can provide you advice on how to implement the portion of pseudo-code you are unsure of.

use a loop to find the last item in the list (i.e. the one which has a "next" link of NULL)

Finding the last item in a linked list is simple. The following function I have crafted below shows how you can obtain a specific node in a linked list at a certain index:

NODE* getNode(NODE* start, int index)
{
    int i;
    for(i = 0; i < index; i++)
    {
        start = start -> next;
    }
    return start;
}

This code can be modified so it instead returns the last node.

NODE* getLastNode(NODE* start)
{
    for(;;)
    {
        start = start -> next;
        if(start -> next == NULL)
            return start;
    }
}

The code above traverses each node in the list until it hits one that has it's connected node as NULL. It then returns that last node.

Now you can call said function above and get the last node.

set the "next" link of this item to point to the new person so the new person becomes the last item in the list (i.e. the new person should have a "next" link of NULL)

I'm sure you know how to do this judging from the code you provided above.