Copying nodes in linked list C

Question

I am trying to duplicate a node in linked list. I am not sure if I am doing it correctly. I tried making test cases but did they were not successful. If some one could tell me if where I went wrong and what I did right, also what is the best way to test my code.

struct node 
{
        int id;
        char side;
        int quantity;
        double price;
};

struct onode 
{
        struct node* data;
        struct onode* next;
        struct onode* prev;
};

struct onode* newNode (struct node* data)

{
    struct node* dataValue  = (struct node*) malloc(sizeof(struct node));
    struct onode* linkedlist = (struct onode*) malloc(sizeof(struct onode));

    linkedlist ->data = (struct node*)malloc(sizeof(data)+1);

    if(dataValue && data)
    {
        *dataValue = *data;
    }
}

I have made changes in my code and added more description on what this function wants. one change : struct node is struct order.

struct order 
{
        int id;
        char side;
        int quantity;
        double price;
};

struct onode 
{
        struct order* data;
        struct onode* next;
        struct onode* prev;
};


/**
 * Returns a new linked list node filled in with the given order, The function
 * allocates a new order and copy the values stored in data then allocate a 
 * linked list node. If you are implementing this function make sure that you
 * duplicate, as the original data may be modified by the calling function.
 */

struct onode* newNode (struct order* data)
{
    struct order* dataValue  = (struct order*) malloc(sizeof(struct order));
    struct onode* linkedlist = (struct onode*) malloc(sizeof(struct onode));

    *dataValue = *data;

    linkedlist ->data = dataValue;

    linkedlist->data->id = dataValue->id;
    linkedlist->data->price = dataValue->price;
    linkedlist->data->quantity = dataValue->quantity;
    linkedlist->data->side = dataValue->side;
    linkedlist->next->prev = NULL;

    return linkedlist;

}

my test case never compiled. so i deleted it to check. I guess i am not sure how to make a test case regarding it. what i did was make a new node (which was going to be the data node) called newnode and passed the node i made. — Eciliptus, Mar 10 '13 at 22:46
You don't need to cast the return value of `malloc` in a C program. — Carl Norum, Mar 10 '13 at 22:50
@CarlNorum I've been told it's good practice by very knowledgeable people. Is this not the norm? — Alex Becker, Mar 10 '13 at 22:52
@AlexBecker i, had changed the the name of the order node to just "node" and forgot to change it there. so order is just "node" — Eciliptus, Mar 10 '13 at 22:54
@AlexBecker, it is definitely not good practice. And it's not idiomatic either. It's required in C++, and appears to be a bad habit people bring along with them when switching from C++ to C. In C the conversions to and from `void *` are implicit, and the explicit cast can hide bugs. Those cases might be strange corner cases, but still... — Carl Norum, Mar 10 '13 at 22:54
You're calling malloc with `sizeof(data) + 1` where you should actually be using `sizeof(struct node)`. `sizeof(struct node*)` and `sizeof(struct node)` are not the same thing. — Tuxdude, Mar 10 '13 at 22:54
Eciliptus - send your prof this link: http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc @AlexBecker - there's plenty of explanation for you there, too. — Carl Norum, Mar 10 '13 at 22:55
@Eciliptus: Make sure to give your professor [this link](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) so he can see how silly he's being by asking you to do that. — dreamlax, Mar 10 '13 at 22:55
@Tuxdude , i decalared linkedlist and dataValue with it, does it not account to the same thing? i thought it did — Eciliptus, Mar 10 '13 at 22:56
`sizeof(struct node*)` returns the size used to store the pointer i.e the `address it holds` which depends on your address space and usually `32 or 64 bits`. Whereas `sizeof(struct node)` returns the number of bytes in memory required to store a `struct node`. — Tuxdude, Mar 10 '13 at 22:58
@Tuxdude thanks for the explaing, also is there a good C programming book you guys recommend? — Eciliptus, Mar 10 '13 at 23:00

score 1 · Answer 1 · answered Mar 10 '13 at 23:06

The crux of your problem is that you are creating two new node objects--one that is dataValue and one that is linkedlist->data. You then copy the passed-in data into dataValue when you really want it to be stored in linkedlist->data.

If you replace

linkedlist ->data = (struct node*)malloc(sizeof(data)+1);

with

linkedList->data = dataValue;

that should get you moving in the right direction.

Mats Petersson · Answer 2 · 2013-03-10T23:37:25.590

This isn't a proper answer, since your code simply doesn't contain the necessary code/explanation to answer your question.

struct onode* newNode (struct node* data)
{
    struct order* dataValue  = (struct node*) malloc(sizeof(struct node));
}

What is struct order* ?? You mean struct node *?

    struct onode* linkedlist = (struct onode*) malloc(sizeof(struct onode));

    linkedlist ->data = (struct node*)malloc(sizeof(data)+1);

The above line seems wrong - sizeof(data) + 1? It's not a string, so adding one is not meaningful, and the size is the size of the pointer, which is probably not what you want. I'm guessing you want linkedList->data = dataValue; instead.

And you need to set the next and prev pointers in linkedList.

    if(dataValue && data)
    {
        *dataValue = *data;
    }

You are probably supposed to return the node.

As Carl pointed out, you shouldn't cast the return value from malloc() - if your compiler complains about it, it's probably because you are compiling the code as C++ rather than as C.

Edit: in the updated code:

*dataValue = *data;

A

linkedlist ->data = dataValue;

linkedlist->data->id = dataValue->id;
linkedlist->data->price = dataValue->price;
linkedlist->data->quantity = dataValue->quantity;
linkedlist->data->side = dataValue->side;

B

linkedlist->next->prev = NULL;

C

A and B does the same thing, so one of them is redundant.

C is almost certainly going to crash your code, since next has not been set to anything. You probably want to use linkedlist->next = NULL and linkedlist->prev = NULL

score 0 · Answer 3 · answered Mar 10 '13 at 23:56

Because struct order is a POD type, things are quite simple.

struct onode* newNode (struct order* data)
{
    struct order* dataValue;
    struct onode* linkedlist;

    If (!data)
    {
        /* Feel free to use any other strategy to
         * handle data == NULL case depending
         * on the needs of your application. */
        return NULL;
    }

    dataValue = malloc(sizeof(struct order));
    linkedlist = malloc(sizeof(struct onode));

    memcpy(dataValue, data, sizeof(*dataValue))

    linkedlist->data = dataValue;

    linkedlist->next = NULL;
    linkedlist->prev = NULL;

    return linkedlist;
}

Copying nodes in linked list C

3 Answers3

Linked

Related