0

I am relatively new to C++ and I have been practicing it for a month since i have a University course, I want to expand my knowledge and learn as much as I can, so that I can write good code. We had an assignment in which we were supposed to create three classes, in the third class we were supposed to write in and group objects from the second class and then do stuff with them, in other words we were asked to create a class named pilot, plane, and then fleet which is made up of planes. The teachers gave this solution and it was quite far fetched from what i envisioned, take a look:

class Fleet {
private:
    struct PlaneList {
        Plane* p;
        PlaneList* next;
PlaneList* node = nullptr , *next_node = nullptr;
public:
void PlaneAdd(Plane* current) {
        if (node == nullptr) {
            node = new PlaneList;
            node->p = current;
            node->next = nullptr;
            next_node = node;
        }
        else next_node->next = new PlaneList;
        next_node = node->next;
        next_node->p = current;
        next_node->next = nullptr;
    }
    };

Now as far as I've gathered we created a list which contains pointers which point to the co-responding plane which is in another class? Also the memory allocation is super weird, i don't really understand it and when I paste this code into my MSVC, the program compiles but it crashes with the message saying that next_node = node->next; is a NULLPTR and it has an access violation? Why am i getting this error? I am totally confused and I'd really appreciate if you could explain it a bit to me. Also I've read that using lists is a terrible performance hit and that using something like vectors is much more efficient.

πάντα ῥεῖ
  • 1
  • 13
  • 116
  • 190
  • You likely don't want to merge Pilots and Planes into one list. They are too different for the aggregation to make any sense. If you have may different types of planes you may want a `Plane` base class for the planes to derive from. – user4581301 Oct 29 '20 at 23:34
  • Recommendation: Separate the fleet logic from the linked list logic. If you have a fleet that contains a linked list, you can test the linked list without the fleet stuff getting in the way and when you're done, you'll have a linked list you can reuse for other jobs later. In general the fewer responsibilities a class has, the easier to test and maintain it is. If you have many small blocks that each do a simple thing, you can test them quickly and easily and then assemble them into larger program with far less fuss than making one monolithic program. – user4581301 Oct 29 '20 at 23:41

1 Answers1

0

Let's take your PlaneAdd function and reformat it a bit:

void PlaneAdd(Plane* current) {
    if (node == nullptr) {
        node = new PlaneList;
        node->p = current;
        node->next = nullptr;
        next_node = node;
    }
    else {
        next_node->next = new PlaneList;
    } 

    next_node = node->next;
    next_node->p = current;
    next_node->next = nullptr;
}

Now we can easily see that the last three statements are not part of the else.

So when node == nullptr is true the node->nextwill be a null pointer when those statements are execute. And the first thing you do is next_node = node->next which turns next_node into a null pointer as well. Which means the next statement will dereference that null pointer and lead to undefined behavior and a likely crash.

Some programmer dude
  • 400,186
  • 35
  • 402
  • 621