-1

I'm working on a class project and this piece of code won't let me delete an instance of a class without throwing a breakpoint error.

The class is Node, I'm trying to build a singly linked list for data structures and algorithms. I'll include the whole program, it isn't long, but the code in question that's causing the problem is in deleteMin(), the delete u.

#include <iostream>
using namespace std;

                    // we create a Node class.
class Node {        // inside this class we hold two pieces of information.
    int x;          // the integer x, which is our data.
    Node* next;     // and the address of the next node.
public: 
    Node(int x0) : x(x0), next(NULL) { } // Here I've set up a constructor that sets x equal to the argument 
                                         // and defaults the next address to NULL.
    bool add(int newValue);              // Here follows our three required functions.
    int deleteMin();            
    int size();
    void printSSL();                     // I also added a printSSL() function so that we can test and see what's going on.
};
                //Originally I wanted to put these inside of the Node class, but then you'd end up with a unique head and tail for every Node.
                //So instead I've left them outside. If you wanted to create multiple SSList's then you'd want to create an object out of these as well.
Node* head;     // The first value in the our SLList.
Node* tail;     // The last value in our SLList.
int n;          // The number of elements in the list.
                // I should mention here that head and tail are set equal to the first value in the SLList in the Main() function below.

                        // Here follows the actual implementation.
                        // I chose to try and simplify things by focusing on the add() function.
                        // If the add function organizes the information, and puts it into the SLList in order, 
                        //then deleteMin() only has to pull the first value.
bool Node::add(int newValue) {                        // add() is a member function of Node and it takes in the newValue were adding to the list.
    Node* u = new Node(newValue);                     // First thing we do is create a new Node using the argument value x. We pass this into a pointer, u.
    if (newValue <= head->x) {                        // Next, we check to see if the new value is less than the head.
        u->next = head;                               // if it is, then our job is done and we just make this new, smaller value, the new head.
        head = u;                                     // we do this by making the initial head equal to the next address in the new Node u.
        n++;                                          // Once we have the address saved, we make u into the new head and increment n.
        return true;                                  // There's no iteration in this case, so this if statement would be O(1).
    }//O(1)
    else {                                            // If the new value is greater than the head, then we have to store it further back in the SLList.
        Node* y = head;                               // This was the hardest part of the whole thing... I solved it by creating two Node pointers, 
        Node* z = head;                               // Both Node pointers are set equal to head, but this is mostly just to ensure that they aren't empty.
        while ((newValue > y->x) && (y != tail)) {    // Then I set a while loop that looks at whether the new value is less than the value in the head.
            z = y;                                    // The while loop continues, moving through the list of values, setting y equal to the next value,
            y = y->next;                              // and using z to keep track of the former value.
        }                                             // The loop exits when we either a) hit the end of the SLList, y == tail, or when the new value is 
        if (y == tail) {                              // smaller than the next value, newValue < y->x. When the loop exits we have to deal with these two
            y->next = u;                              // scenarios separately. If we reached the end of our list, then adding the new value is as simple as 
            tail = u;                                 // setting y->next equal to u, then we make u into the new tail.
        }                                             // if we didn't reach the end of the list, then we have to set u inbetween z and y. This was really 
        else {                                        // the only reason I needed z. I needed to be able to update the address of the previous Node, and 
            z->next = u;                              // I also needed to have the address of the next Node, this way I could slip u inbetween the two.
            u->next = y;                              // Worst case scenario, this function runs through the entire SLList and then adds the value at the end.
        }                                             // I could have shaved off some time by asking if(newValue > tail->x) then run the z->next=u; and u->next=y; after
        n++;                                          // after that, but that throws an error becauset ail is a NULL pointer, which is bull@*#!
        return true;                                  // because I'm not dealing the tail->next, all I care about is tail->x which isn't NULL.
    }//O(n)                                           // But because this is an SSList and not a DLList I have no way of going to the n-2 element.
}//O(max(1, n))                                       // When that's done, we just increment n and leave the function.
                                                      // Considering that the worst case scenario, when x > tail->x, takes us through the whole SLList.
                                                      // I'm going to say that this add function is O(n).
int Node::deleteMin() {                               // The deleteMin() function starts by checking whether or not the 
    int x = head->x;
    Node* u = head;
    head = head->next;
    delete u; // I have to figure out what the hells going on right here, why can't I delete this?
    return x;
}

int Node::size() {
    cout << n + 1 << endl;
    return n + 1;
}

void Node::printSSL() {
    Node* u = head;
    cout << "Head:";
    for (int i = 0; i <= n; i++) {
        cout << i << ":(" << u->x << ", " << u->next << ")  ";
        u = u->next;
    }
    cout << " Tail" << endl;
}

int main()
{
    Node one(1);
    head = &one;
    tail = &one;
    one.printSSL();
    one.deleteMin();
}
  • 1
    You didn't `new` `head` - it's refering to `&one`, so why are you trying to `delete`? – UnholySheep Jul 20 '22 at 20:41
  • 1
    You can't `delete` it because you didn't `new` it in the first place. – john Jul 20 '22 at 20:41
  • Try `head = tail = new Node(1);` instead. – john Jul 20 '22 at 20:42
  • But I did NEW it when I added it to the SLList here: Node* u = new Node(newValue); – Westcott Louden Jul 20 '22 at 20:43
  • @WestcottLouden So where in your code are you calling `add`? – john Jul 20 '22 at 20:43
  • The only reason I'm looking to delete this is I don't want the instructor to say that I left an object in memory. Will it disappear when it leaves scope? – Westcott Louden Jul 20 '22 at 20:44
  • If I add anything in Main(). I left that out cuz its just there for testing but in Main() I have a bunch of functions calls like this: one.printSSL(); one.add(2); one.printSSL(); one.add(27); one.printSSL(); one.add(27); one.printSSL(); one.add(1); – Westcott Louden Jul 20 '22 at 20:45
  • 1
    @WestcottLouden You cannot have a linked list where some items are allocated with `new` but some are not. Precisely because you will not know which ones are to be deleted and which aren't. You should allocate all your nodes with new. The problem isn't that you are trying to delete. The problem is that you aren't using new. – john Jul 20 '22 at 20:46
  • It would seem that you designed `head` to sometimes point to a `new Node` and sometimes point to `&one`. That suggests a design mistake. If you aren't tracking whether the pointer came from `new` then you aren't tracking whether you can or should use `delete`. – Drew Dormann Jul 20 '22 at 20:47
  • @john Ah! That makes sense! In Main() I just create an instance of the object but then the add function is using NEW. – Westcott Louden Jul 20 '22 at 20:47
  • @WestcottLouden The fact that you have to declare a node object just so you can add another node shows that your code has a design flaw. – john Jul 20 '22 at 20:49
  • @WestcottLouden What you should do is create a List class, and put head, tail, add, deleteMin, size and printSSL in that List class. These are not node functions, they are list functions. – john Jul 20 '22 at 20:50
  • @john You might've just saved me some marks LOL. Thanks. – Westcott Louden Jul 20 '22 at 20:52
  • You already recognized that you're getting into trouble with the class design, since you're adding member variables to a class where they don't belong. A better choice of data design for a data structure would be something like this: `struct List{ struct Node {int data; Node* next; }; size_t size; Node* head; Node* tail; bool add(int newValue); // Here follows our three required functions. int deleteMin(); int size(); void printSSL(); };` (with different visibility modifiers of course...) – fabian Jul 20 '22 at 20:55
  • No I totally agree, I was spacing on whether or not I could put the Node class inside of the List class. I haven't touched C++ since I took it as my first programming class last year so LOL. I'm still pretty noob obviously. But yea that makes sense. I gotta use this website more. – Westcott Louden Jul 20 '22 at 21:06
  • You shouldn't use new at all. Use `std:: unique_ptr` or `std::shared_ptr`. And please [don't use using namespace std](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice) – infinitezero Jul 20 '22 at 21:10
  • Don't update your question in a way that renders all of the previous comments and answers meaningless. If the answer below is satisfactory, then accept it and be done. – Beta Jul 20 '22 at 22:21
  • Okay I found the old code. – Westcott Louden Jul 20 '22 at 22:25
  • *"I'll include the whole program"* -- bad idea. Questions should have a **minimal** reproducible example, not an entire program. In this case, you reduced your main function to calling just three functions: a `Node` constructor, `printSSL()`, and `deleteMin()`. That means you can get rid of all the other functions (maybe replace them with a comment indicating what has been omitted). *Why is this a good idea? Well, for one thing it removes your only use of `new`, making the remaining `delete` highly suspicious... In other words, it helps you (and us) focus on what is really relevant.* – JaMiT Jul 21 '22 at 03:15

2 Answers2

1

You declared an object of the type Node

Node one(1);

You may not apply the operator delete to a pointer to the object because the object was not allocated dynamically. It has automatic storage duration.

Pay attention to that it is a bad idea when functions depend on global variables. For example you will be unable to define two lists in your program.

What you need is to define a class named something like List the following way

class List
{
private:
    Node *head = nullptr, *tail = nullptr;

public:
    // special member functions and some other member functions;
    void clear(); 
    ~List() { clear(); }
};

and to allocate nodes dynamically that will be inserted in the list.

The destructor and the function clear will delete all the allocated nodes in the list.

class Node also should be defined as a nested class of the class List.

For example the function clear can be defined the following way

#include <functional>

//...

void List::clear()
{
    while ( head )
    {
        delete std::exchange( head, head->next );
    }

    tail = nullptr;
}
Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
  • You should add `List(List&&) = delete; List& operator=(List&&) = delete;` to your class for the time being to avoid issues with move/copy operations. Those would necessarily need to be implemented manually to manage resources appropriately. Personally I'd also make `Node` a nested type in `List`... – fabian Jul 20 '22 at 21:01
  • @fabian You may add any functions instead of my comment // some member functions; as you like.:) – Vlad from Moscow Jul 20 '22 at 21:02
  • I just updated the original post with my new code. If you guys see anymore room for improvement let me know! I much prefer this now. Only complaint is that the Add function got a little more complicated to accommodate that empty SLList. – Westcott Louden Jul 20 '22 at 22:14
  • @WestcottLouden It is a bad idea because that will only confuse readers of the question and answers. You should restore the original code in the question and select the best answer. If you have some problems with your new code then ask a new question. – Vlad from Moscow Jul 20 '22 at 22:19
  • Oh shoot, too late. Its gone. I'm new to Stackoverflow so I'll keep that in mind for next time. – Westcott Louden Jul 20 '22 at 22:21
  • NVM its put back. – Westcott Louden Jul 20 '22 at 22:25
-1
#include <iostream>
using namespace std;

class SLList {                                             // SLList is the object that holds the entire list.
public:                                                    // The Node class lives inside the SLList class.
    struct Node {
        int data;
        Node* next;
        Node(int x0) : data(x0), next(NULL) {}
        Node() : data(NULL), next(NULL) {}
    };
    Node* head;
    Node* tail;
    int n;
    SLList() : n(0) {
        Node* initial = new Node();
        head = initial;
        tail = initial;
        cout << "You've created a new SSList" << endl;
    }
    bool add(int newValue);             
    int deleteMin();
    int size();
    void printSSL();
};

bool SLList::add(int newValue) {                          // 
    if (n == 0) {
        head->data = newValue;
        n++;
        return true;
    }
    else {
        Node* u = new Node(newValue);
        if (newValue <= head->data) {                     // 
            u->next = head;                               // 
            head = u;                                     // 
            n++;                                          // 
            return true;                                  // 
        }//O(1)
        else {                                            // 
            Node* y = head;                               // 
            Node* z = head;                               // 
            while ((newValue > y->data) && (y != tail)) { // 
                z = y;                                    // 
                y = y->next;                              // 
            }                                             // 
            if (y == tail && newValue > y->data) {
                y->next = u;                              // 
                tail = u;                                 // 
            }                                             // 
            else {                                        // 
                z->next = u;                              // 
                u->next = y;                              // 
            }                                             // 
            n++;                                          // 
            return true;
        }                                 
    }//O(n)                                               // 
}//O(max(1, n))                                           // 

int SLList::deleteMin() {                              
    int x = head->data;
    Node* u = head;
    head = head->next;
    delete u;
    n--;
    return x;
}//O(1)

int SLList::size() {
    cout << n + 1 << endl;
    return n + 1;
}//O(1)

void SLList::printSSL() {
    Node* u = head;
    cout << n << " Nodes|" << "Head:";
    for (int i = 0; i < n; i++) {
        cout << i << ":(" << u->data << ", " << u->next << ")  ";
        u = u->next;
    }
    cout << " Tail" << endl;
}//O(n)

int main() {
    SLList* one = new SLList;
    one->printSSL();
    one->add(30);
    one->printSSL();
    one->add(20);
    one->printSSL();
    for (int i = 0; i < 7; i++) {
        int x = rand() % 50;
        one->add(x);
        one->printSSL();
    }   
    for (int i = 0; i < 9; i++) {
        one->deleteMin();
        one->printSSL();
    }
 }
  • I didn't see this Answer part before. But yea that seems to work much better. Thanks guys! – Westcott Louden Jul 20 '22 at 22:27
  • 1
    Code only answers are unlikely to be of use in the future. You should add an explanation of what you did and why the changes help. – JaMiT Jul 21 '22 at 03:17