0

i have a full linked list with data in it, what i want is to fill another linked list with the same data but with a condition, so let say that this is the linked list :

char cl; 
int time; 
int lng;

C 0 1

D 0 2

B 2 1

A 2 2

i wan to copy from this list to a new empty one but only if (time>0), so the new one will be like this :

B 2 1

A 2 2

i have tried this code but it doesn't work :

void insert(const node * n )// this where i put the pointer of the full node
{
    node *head=n;
    node *sec; // new node
    sec=(node*)malloc(sizeof(node)); 

    do{
        if(head->time>0){
            strcpy(sec->cl,head->cl);
            sec->time= head->time;
            sec->lng= head->lng;
            head=head->next;
            sec=sec->next;
        }
        else 
            head=head->next;
    }while(head!=NULL);
    print(sec) // this will print the new node
}

Help me please. Thank you

Xan
  • 74,770
  • 16
  • 179
  • 206
alex
  • 79
  • 2
  • 14
  • 'new' is a reserved word in c++ and if the compiler understands both C and C++, it will have a problem. – user3629249 Jan 25 '15 at 18:12
  • 2
    How did you create the original linked list? Why can't you use the functions that you have used to build that list? – M Oehm Jan 25 '15 at 18:38
  • Don't use strcpy! strcpy is used to copy strings (null-terminated), but you only have a single char. Use `sec->cl = head->cl;` or it will break your code. – maja Jan 25 '15 at 18:40
  • Or did you forget the * in `char *cl;`? If yes, you forgot to allocate memory for the string. – maja Jan 25 '15 at 18:41
  • 1
    You only allocated memory for the first item in `sec` and not subsequent ones, and `sec=sec->next` will cause undefined behaviour, because the field has not been initialised. – Weather Vane Jan 25 '15 at 18:41
  • `it doesn't work` - Does it compile? Does the program crash? Does it produce wrong results? – maja Jan 25 '15 at 18:42
  • 1
    Remove the `else` before `else head=head->next;` – Weather Vane Jan 25 '15 at 18:44
  • 1
    `while(head!=NULL)` should be at the top of the loop, not the tail. – Weather Vane Jan 25 '15 at 18:45
  • @user3629249 No it's not a problem when compiling C code. If you write C code and compile it as C++ code, then it's a problem. But it has nothing to do with a compiler being able to compile both. A C/C++ compiler compiling C code as C will have no problems with `new` or the compiler is broken. – Jite Jan 25 '15 at 19:05

2 Answers2

2

I combined all the suggestions from the comments as well as some additional fixes. Here's the resulting code:

const node* insert(const node * head)
{
    node *sec = NULL;  // start of the new, second list
    node *last = NULL; // points to the last inserted node

    while(head!=NULL){
        if(head->time > 0){
            node* newsec=(node*)malloc(sizeof(node)); 
            newsec->cl = head->cl;
            newsec->time = head->time;
            newsec->lng = head->lng;
            newsec->next = NULL;
            //Add the new node to the list:
            if(last == NULL){ //This is the first element in the new list
               sec = newsec;
            }else{
               last-> next = newsec;
            }
            last = newsec;
        }
        head=head->next;
    }
    print(sec); // this will print the new node
    return sec;
}

Your mistakes:

  • Wrong memory allocation (You only allocated memory once)
  • strcpy is not needed (char's don't require a string-copy)
  • while must be at the beginning of the loop (your code would fail if the given list is empty)
  • missing semicolon
  • wrong concatination of the new list
  • wrong const-correctness (Missing const in node *head=n;)
  • the internal head-variable is not neccessary (And the parameter-naming n is also not ideal. If you name it "start"/"head"/"begin", the comment wouldn't be neccessary)

Another suggestion: Use an upper case name for your struct, since it makes it easier to distinguish types from variables (should be Node, not node).

Note that you might want to remove the const from the return value type.

maja
  • 17,250
  • 17
  • 82
  • 125
  • 1
    `node newsec` should be `node *newsec`, and don't cast the result of malloc. – Jite Jan 25 '15 at 18:53
  • @Jite I guess that OP is using a C++ compiler, which requires a cast. – maja Jan 25 '15 at 18:56
  • Why would you guess that? It's C code and to compile C code you use a C compiler, even if the compiler can compile C++. There is no need, and its indeed not recommended, to cast the result of malloc in C. The question is tagged C. – Jite Jan 25 '15 at 18:58
  • 1
    Shouldn't the routine return `sec` as the head of the copied list? – M Oehm Jan 25 '15 at 18:58
  • @Jite C++ requires the result of malloc to be casted (http://stackoverflow.com/questions/3477741/why-does-c-require-a-cast-for-malloc-but-c-doesnt) - and although the question is tagged as C, I think that the OP is actually using a C++ compiler since he used `new` in his initial post. If he is in fact using a C compiler, he can remove the cast. (I wanted to ensure that my answer works on his setup.) – maja Jan 25 '15 at 19:08
  • @maja The code would have to be compiled as C++ code for it to be a problem, independent of compiler capabilities. – Jite Jan 25 '15 at 19:14
-1
// Note: a better function name might be: printNodesWithTime

void insert(const node * pOld )
{

    // note: since nothing is done with the new linked list,
    //       there is no need to actually create it

    while( NULL != pOld )
    {

        if(pPld->time > 0)
        {
            print(pOld) // this will print the node
        }

        // step to next old node
        pOld = pOld->next;

    } // end while
} // end function: insert
user3629249
  • 16,402
  • 1
  • 16
  • 17