Andrew Schmitz
Andrew Schmitz

Reputation: 17

Linked list doesn't work

I have the following code, when I add nodes why does it reset all nodes to the most recent nodes data? Assuming everything else in the codes works correctly, parsing etc. can anyone identify a problem with the list. See output at the bottom.

typedef char* string;
typedef struct linked_list listType;
typedef struct linked_list* linked_list;
typedef struct node NodeType;
typedef struct node* Node;

typedef enum boolean{
    true = 1,
    false = 0
}boolean; 

struct node{
    Node next;//next node in the list
    string *transData;//listAdd--->string data[]
};

struct linked_list{
    Node head;//first node in the list
    int size;//size of list, number of nodes in list
};

boolean add(linked_list list, string *data)
{
    boolean retval = false;//default retval is false
    Node nod;//node to add
    nod = (Node)malloc(sizeof(NodeType));
    nod->transData = data;
    nod->next = NULL;

    //list size is 0
    if(list->head == NULL)
    {
        list->head = nod;
        printf("First Node added: '%s'\n", nod->transData[0]);fflush(stdout); 
        retval = true;
    }

    //nodes already in list
    else
    {
        printf("List with nodes already\n");
        fflush(stdout); 
        Node node = list->head;

        //iterating through nodes
        while(node->next != NULL)
        {
            node = node->next;
        }//while

        node->next = nod;
        printf("Node added: '%s'\n", nod->transData[0]); fflush(stdout);
        retval = true;
    }

    list->size+=1;
    return retval;//success
}

/**
 * Returns the size of the list.
 */
int listSize(linked_list list)
{
    return list->size;
}

/**
 *Can only print with 2 or more elements
 */
void printList(linked_list list)
{
    int i=0;
    Node nod = list->head;
    int length = listSize(list);
    printf("ListSize:%d\n",listSize(list));
    fflush(stdout);

    while(nod->next != NULL)
    {
        printf("Node %d's data: %s\n", i, nod->transData[0]);
        fflush(stdout);
        nod = nod->next;
        i++;
    }
}//printlist

Output:

trans 5 5  
Argument 1: trans  
Argument 2: 5  
Argument 3: 5  
Last spot(4) is: (null)  
name of command: trans  
ID 1  
First Node added: 'trans'  
ListSize:1
>>check 4  
Argument 1: check  
Argument 2: 4  
Last spot(3) is: (null)  
name of command: check  
ID 2  
List with nodes already  
Node added: 'check'  
ListSize:2  
Node 0's data: check  
>>trans 4 4  
Argument 1: trans  
Argument 2: 4  
Argument 3: 4  
Last spot(4) is: (null)  
name of command: trans  
ID 3  
List with nodes already  
Node added: 'trans'  
ListSize:3  
Node 0's data: trans  
Node 1's data: trans  
>>check 5  
Argument 1: check  
Argument 2: 5  
Last spot(3) is: (null)  
name of command: check  
ID 4  
List with nodes already  
Node added: 'check'  
ListSize:4  
Node 0's data: check  
Node 1's data: check  
Node 2's data: check  

Upvotes: 0

Views: 279

Answers (3)

(The OP tells me that the nodes need to have arrays of strings, so the pointer-to-a-pointer thing is actually correct).

Anyhow, it looks like your problem is that every node has a pointer to the same buffer. You just keep copying different strings into the same buffer, and then you assign the address of a pointer to that buffer to each new node.

First, I'd get rid of those typedefs. I don't know how much clarity they're really adding.

Next, you need to allocate new storage for each string. The standard library strdup() function is a handy way to do that:

//  Turns out it's a fixed-size array, so we can blow off some memory 
//  management complexity. 
#define CMDARRAYSIZE 21

struct node {
    Node next;
    char * transData[ CMDARRAYSIZE ];
};

//  Node initialization:

int i = 0;

struct node * nod = (struct node *)malloc( sizeof( struct node ) );
//  Initialize alloc'd memory to all zeroes. This sets the next pointer 
//  to NULL, and all the char *'s as well. 
memset( nod, 0, sizeof( struct node ) );

for ( i = 0; i < CMDARRAYSIZE; ++i ) {
    if ( NULL != data[ i ] ) {
        nod->transData[ i ] = strdup( data[ i ] );
    }
}

...but then be sure that when you free each node, you call free( nod->transData[n] ) for every non-NULL string pointer in nod->transData. C is not a labor-saving language.

Upvotes: 3

Tim
Tim

Reputation: 9172

Not the main issue, but your printList won't print out the last element.

Instead of while(nod->next != NULL) use while(nod != NULL)

Upvotes: 1

jdehaan
jdehaan

Reputation: 19928

Your issue is probably due to pointer copying. You are likely referencing the same buffer in all your nodes. Try to make a copy of your string data with strdup for example.

Upvotes: 1

Related Questions