Adding Problem

[merta]

This test program for the function add, does not work well. I think, after it creates a node for circular linked list, it is not create or add another node in the list in alphabetical order by task name. But ı donot found actually where ı mistake. Could you please help?

#include <iostream>
using namespace std;

struct Node{
    int id;
    string task;
    string person;
    string date;
    Node *next;
};

void add(Node *&);

int main()
{
    char command;
    Node *lookedNode = NULL;
    cout << "For create or create a node, enter a chraacter except for E (E is to end the program) : " << endl;
    cin >> command;

    while(command != 'E'){
    add(lookedNode);
    cout << lookedNode->task << endl;

    cout << "For create or create a node, enter a chraacter except for E (E is to end the program) : " << endl;
    cin >> command;
    }
    return 0;
}

void add(Node *&firstLooked)
{
    string command;
    string taskName;
    string personName;
    string time;
    Node *current = new Node;
    Node *prev = new Node;
    Node *additive = new Node;
    current = firstLooked;

    getline(cin, command);

    cout << "Please enter the task: ";
    getline(cin, taskName);
    additive->task = taskName;
    cout << "Please enter name of person who does the task: ";
    getline(cin, personName);
    additive->person = personName;
    cout << "Please enter the deadline like this: dd.mm.year : ";
    getline(cin, time);
    additive->date = time;

    if(firstLooked == NULL){
        firstLooked = additive;
        firstLooked->next = firstLooked;
        additive->id = 1;
    }
    else{
        cout << firstLooked->id << endl;
        if( (firstLooked->next) == (firstLooked) ){

            cout << firstLooked->id << endl;
            if( (firstLooked->task) < (additive->task)){
                firstLooked->next = additive;
                additive->next = firstLooked;
                additive->id += 1;
            }
            else{
                additive->next = firstLooked;
                firstLooked->next = additive;
                firstLooked = additive;
                firstLooked->next = additive;
                additive->id += 1;
            }

        }
        else{
            prev->next = current;

            while( (current->task) < (additive->task) ){
                current = current->next;
                prev->next = current;
                additive->id += 1;

                if((current->next->task) == (current->task) ){
                    break;
                }

            }

            prev->next = additive;
            additive->next = current;

        }
    }

    delete current;
    delete prev;
    delete additive;
    cout << endl;
}

[Disch]

The overall flow of this code doesn't really make sense. There are a lot of problems that are difficult to pick out one-by-one. It seems to me that you have some big misunderstandings of how new/delete and heap memory work.

So rather than point out individual mistakes, I'm going to try and give an overview of what is going on here.

Let's start with some basics:

int* joe = nullptr;  // nullptr is the C++11 standard version of "NULL"

/*
  Here, 'joe' is a pointer.  A pointer merely contains an address to somewhere
else in memory.  Currently, 'joe' contains a null address, which basically means
it is pointing to nothing.
*/

joe = new int;

/*
  Here, "new int" will create a new, NAMELESS int on the heap.  Let's call this
nameless int 'foo'.  So new creates this 'foo' int, and then returns the address
of it.  That address gets assigned to our pointer, 'joe'

Conceptually this is similar to doing this:

  int foo;
  joe = &foo;

The only difference is, 'foo' is created dynamically on the heap, and does not
get automatically cleaned up.
*/

*joe = 5;   // since 'joe' points to 'foo', this assigns 5 to foo.  As if
    //  we were saying "foo = 5;"


// Here, let's create another pointer:
int* sam = joe;

/*
   Now here... we are assigning the contents of joe to sam.  This means that
whatever address is currently in 'joe' gets copied to 'sam'

  Since 'joe' contains the address of foo... this means 'sam' will also contain
the address of foo.  So at this point... both 'joe' and 'sam' point to foo.
*/

*sam = 7;  // This will assign 7 to foo as you'd expect... since sam points to foo

//   now...
// cleanup:
delete joe;

/*
   Contrary to what this says... WE ARE NOT DELETING JOE.  We are deleting
whatever joe points to.  Since joe points to foo... this will DELETE FOO.

   We can't say delete foo; because 'foo' is not a real name.  Remember that
things created with 'new' are nameless, so we have to refer to them by their
address.

   At this point... after that delete... foo no longer exists!
*/

// try to assign something:
*sam = 10;  // EXPLODE!!! BAD BAD BAD

/*
   Here, we are trying to assign a value of 10 to whatever 'sam' points to.  If
you remember, 'sam' points to foo.  BUT WE DELETED FOO, so it no longer
exists.  Therefore, sam now points to garbage, and we are attempting to write
to some random place in memory.  This is bad bad bad bad.
*/

Can you follow the logic there? Do you see the problem?


This is one of the things you are doing in your code.

In your 'add' function, you are creating 3 nodes with new... but then at the end of it you are attempting to delete all of them!

Furthermore, you are overwriting your pointers with new addresses shortly after you created a new node with 'new'.





So now that some conceptual points are out of the way... let's look at what you're actually doing:

    Node *lookedNode = NULL;
    //...
    add(lookedNode);

I don't know if this was intentional... but you never created a node. Instead, you are just passing a null pointer into your add function. I see it takes the pointer by reference... so maybe it is add's responsibility to create the node?

Assuming this was intentional... and that you want a null pointer passed in... let's look at what add() is doing:

void add(Node *&firstLooked)
{
    // ...
    Node *current = new Node;  // create a new nameless node...  'A'
    Node *prev = new Node;  // new nameless node 'B'
    Node *additive = new Node;  // new nameless node 'C'

    // all that was fine... but then you do this:
    current = firstLooked;  // current no longer points to A, but instead points
        // to whatever firstLooked pointed to.  Since firstLooked was null,
        // this means 'current' will now also be null.

    // This is a problem because now 'current' points to nothing.. AND you
    //  no longer have anything pointing to 'A', which means it is impossible
    //  to delete A, which means you have a memory leak.

    // to recap.. at this point:
    //   prev = points to B
    //   additive = points to C
    //   current = null
    //   firstLooked = null


    // ...

    // all this stuff will modify the parts of 'C'
    //   this is all fine:
    additive->task = taskName;     // C.task = taskName
    additive->person = personName;  // C.person = personName
    additive->date = time;  // C.date = time

    //  ...
    if(firstLooked == NULL){  // firstLooked is null here.  So this will run
        firstLooked = additive;   //  now firstLooked points to whatever additive
           // points to.  Since additive pointed to 'C', this means firstLooked
           //  now points to C
        firstLooked->next = firstLooked;  // since firstLooked points to C, this
           // means that firstLooked->next is really C.next
           // So this means C.next is pointing to C
           // This is probably a mistake... since this would create an infinite loop
           //  because the "next" node is referring to itself.
        additive->id = 1;  // C.id = 1
    }

    // to recap.. at this point:
    //   prev = points to B
    //   additive = points to C
    //   current = null
    //   firstLooked = points to C
    //   C.next = points to C

    // ...

    // Then at the end you start deleting stuff:
    delete current;  //  current is null... so this does nothing.  Nothing is deleted
       // when you attempt to do a delete with a null pointer.

    delete prev;   // prev points to B, so this deletes B
    delete additive;  // additive points to C, so this deletes C

    /*
NOTE the problem here!  Just like my joe/same example above... you just
deleted the 'C' node.  However, firstLooked is still pointing to it!  This means
firstLooked is now pointing to GARBAGE and it must not be accessed.
    */

EDIT: clarified a bit regarding C.next

[merta]

Thank you very much. You are right, ı have some big misunderstandings of how new/delete and heap memory work, i see this thanks to you. Then,

    Node *lookedNode = NULL;
    //...
    add(lookedNode);

it was intentional, ı try to create a pointer that points to circular linked list, but at first time(before calling add function) as there is not a linked list(ı think), lookedNode points to nothing.

About C.next: I try to create a circular linked list, so C.next is pointing to C. I mean when ı add one node in the list, this node.next points to itself. Am ı wrong about when a circular linked list has one node, this one node.next needs to point to itself?

[Disch]

it was intentional, ı try to create a pointer that points to circular linked list, but at first time(before calling add function) as there is not a linked list(ı think), lookedNode points to nothing.

Okay. That makes sense. =)

About C.next: I try to create a circular linked list, so C.next is pointing to C. I mean when ı add one node in the list, this node.next points to itself. Am ı wrong about when a circular linked list has one node, this one node.next needs to point to itself?

Okay I didn't realize it was circular. Yes, if there is 1 node in a circular linked list, it would have itself as the next node.