Debugging linked list pointers code: segmentation fault

Question

So the following code:

/*
 * For your reference:
 *
 * SinglyLinkedListNode {
 *     int data;
 *     SinglyLinkedListNode* next;
 * };
 *
 */
SinglyLinkedListNode* insertNodeAtTail(SinglyLinkedListNode* head, int data) {
  SinglyLinkedListNode* temp = head;
  while (temp != NULL) {
    temp = temp->next;
  }
  SinglyLinkedListNode* temp1;
  temp1->data = data;
  temp1->next = NULL;
  temp->next = temp1;
  return temp;
}

So, basically I want to add "data" at the end of the linked list "head" and return the updated list. So where is the fault?

Edit: Okay I got the first mistake. But even if I replace temp!=NULL with temp->next!=NULL in the loop conditional still there's this error

Answer 1

This function

SinglyLinkedListNode* insertNodeAtTail(SinglyLinkedListNode* head, int data) {
SinglyLinkedListNode* temp=head;
while(temp!=NULL){
    temp=temp->next;
}
SinglyLinkedListNode* temp1;
temp1->data=data;
temp1->next=NULL;
temp->next=temp1;
return temp;
}

does not make sense. After this loop

while(temp!=NULL){
    temp=temp->next;
}

the pointer temp is equal to NULL. So this statement

temp->next=temp1;

invokes undefined behavior.

The pointer temp1 was not initialized. So again these statements

temp1->data=data;
temp1->next=NULL;

invoke undefined behavior.

The user of the function does not know whether the returned pointer is the head pointer or the last pointer of the list. So it is unclear whether to assign the returned pointer to the head pointer or just to ignore the returned value.

The function can look the following way.

void insertNodeAtTail( SinglyLinkedListNode * &head, int data ) 
{
    SinglyLinkedListNode **current = &head;

    while ( *current != nullptr ) current = &( *current )->next;

    *current = new SinglyLinkedListNode { data, nullptr };
}

If in main you defined the pointer to the head node like

SinglyLinkedListNode *head = nullptr;

then a function call will look like

insertNodeAtTail( head, some_data );

Another definition of the function can look the following way

SinglyLinkedListNode* insertNodeAtTail( SinglyLinkedListNode *head, int data ) 
{
    SinglyLinkedListNode *new_node = new SinglyLinkedListNode { data, nullptr };

    if ( head == nullptr )
    {
        head = new_node;
    }
    else
    {
        SinglyLinkedListNode *current = head;

        while ( current->next != nullptr ) current = current->next;

        current->next = new_node;
    }

    return head;
}

In this case if in main you defined the pointer to the head node like

SinglyLinkedListNode *head = nullptr;

then the function call will look like

head = insertNodeAtTail( head, some_data );

Between these two function definitions the first function definition is preferable because there is no need to remember to assign the returned pointer to the head node.

Bear in mind that if you have a singly-linked list and want to append new nodes to the tail of the list ten it is better to define two-sided singly-linked list. In this case the list definition can look like

class SinglyLinkedList
{
private:
    struct Node
    {
        int data,
        Node *next;
    } *head = nullptr, *tail = nullptr;

public:

    SinglyLinkedList() = default;
    void insertNodeAtHead( int data );
    void insertNodeAtTail( int data );
    // other member functions;
};

Answer 2

You have to allocate memory for the node. Remember to clean up the allocated memory. For each call to new you need a call delete. Therefore I prefer smart pointers.

After your loop temp contains NULL. You can't dereference a null pointer.

SinglyLinkedListNode* insertNodeAtTail(SinglyLinkedListNode* head, int data) {
    SinglyLinkedListNode* temp1 = new SinglyLinkedListNode;
    temp1->data = data;
    temp1->next = nullptr;
    if (!head) {
        head = temp1;
        return head;
    }
    SinglyLinkedListNode* temp = head;
    while(temp->next){
        temp = temp->next;
    }
    temp->next = temp1;
    return temp;
}

Answer 3

It crashes here also:

SinglyLinkedListNode* temp1;
temp1->data=data;

when you reach the end of the loop while(temp!=NULL){ , here temp is NULL. below you are using a statement like this temp->next=temp1; this causes crash