Search in a Doubly-Linked List founds members multiple times

Question

I'm working through the exercises from PPPC++ and I have a List class that holds multiple gods with their attributes.

Ex: {Thor, Norse, Chariot, Mjolnir} or {Hera, Greek, chariot, pomegranate} where Thor is Norse god and Hera is a Greek god.

I'm trying to write the code to find the pointers that point to all the gods that are Greek.

And I don't get why Hera is found twice and Ares if found 4 times. What's wrong? Thanks!

found 0x5586b65353f0 Poseidon
found 0x5586b6535350 Athena
found 0x5586b65351d0 Hera
found 0x5586b65351d0 Hera
found 0x5586b6534f50 Ares
found 0x5586b6534f50 Ares
found 0x5586b6534f50 Ares
found 0x5586b6534f50 Ares
found 0x5586b6534eb0 Zeus

The code is:

#include <iostream>
#include <stdexcept>
#include <string>
using namespace std;

struct God
{
    God(const string &n, const string &m, const string &v, const string &w)
        : name{n}, mythology{m}, vehicle{v}, weapon{w} {}

    string name;
    string mythology;
    string vehicle;
    string weapon;
};

class Link
{
public:
    God god; 
    Link(const string &n, const string &m, const string &v, const string &w, Link *p = nullptr, Link *s = nullptr)
        : god{n, m, v, w}, prev{p}, succ{s} {}
    Link *insert(Link *n);                 // insert n before this object
    Link *find_mythology(const string &s); // find s in list

    Link *next() const { return succ; }
    Link *previous() const { return prev; }

private:
    Link *prev;
    Link *succ;
};

Link *Link::insert(Link *n) // insert n before this object; return n
{
    if (n == nullptr)
        return this;
    if (this == nullptr)
        return n;
    n->succ = this;     // this object comes after n
    if (prev)           // if prev of this (object) is not zero - meaning there's a Link object before this object (or p)
        prev->succ = n; // perv->succ
    n->prev = prev;     // this object’s predecessor becomes n’s predecessor
    prev = n;           // n becomes this object’s predecessor
    return n;           // returns n(the new element) which is before the top node
}
Link *Link::find_mythology(const string &s) // find s in list;
{
    Link *p = this;
    while (p)
    {
        if (p->god.mythology == s)
            return p;
        p = p->succ; // move to the next node
    }
    return nullptr; // return nullptr for “not found”
}
void print_all(Link *p)
{
    while (p)
    {
        cout << " " << p->god.name << ", " << p->god.mythology << ", " << p->god.vehicle << ", " << p->god.weapon;
        if (p = p->next()) // moved to the next node
            cout << "\n";
    }
}

int main()
{
    Link *all_gods = new Link{"Zeus", "Greek", "chair", "lightning"};
    all_gods = all_gods->insert(new Link{"Ares", "Greek", "wings", "sword"});
    all_gods = all_gods->insert(new Link{"Odin", "Norse", "Sleipner", "Gungnir"});
    all_gods = all_gods->insert(new Link{"Thor", "Norse", "Chariot", "Mjolnir"});
    all_gods = all_gods->insert(new Link{"Freia", "Norse", "chariot", "Brisingamen"});
    all_gods = all_gods->insert(new Link{"Hera", "Greek", "chariot", "pomegranate"});
    all_gods = all_gods->insert(new Link{"Tyr", "Norse", "chariot", "spear of justice"});
    all_gods = all_gods->insert(new Link{"Athena", "Greek", "chariot", "thunderbolt"});
    all_gods = all_gods->insert(new Link{"Poseidon", "Greek", "the sea", "trident"});

    print_all(all_gods); // cout the type of the 1st element
    cout << "\n\n";

//while (all_gods)
//{
//    Link *p = all_gods->find_mythology("Greek"); // this returns a pointer where it finds a Norse
//     cout << "found " << p << ' ' << p->god.name << '\n';
//   all_gods = all_gods->next();
//}

Link *p = all_gods;
while (p)
{
    if (p->god.mythology == "Greek")
        cout << "found " << p << ' ' << p->god.name << '\n';
    p = p->next();
}
delete[] p;
// here I will still use all_gods before deleting it below
delete all_gods;

print_all(all_gods);
cout << "\n\n";
print_all(p);
cout << "\n\n";
}

Edit

I changed the while loop in main() and now it does what I wanted.

Answer 1

Your code looks good (though not very OO).

It also seems to be working the way I would expect:

// When you start the list is:
//    Poseidon : Athena : Tyr : Hera ......
while (all_gods)
{
    // So first time threw this loop you find: Poseidon
    Link *p = all_gods->find_mythology("Greek");
    cout << "found " << p << ' ' << p->god.name << '\n';

    // Now you are altering the list (and leaking an object.
    // but thats another question).
    //
    
    all_gods = all_gods->next();
    // But you dropped "Poseidon" off the front so the list is now:
    // Athena : Tyr : Hera ......
}

So your second time around the loop you print Athena. Then you drop Athena from the list so the list is Tyr : Hera .......

So your third time around the loop you will print Hera (first Greek god). But then you drop Tyr so the list is Hera .......

So the fourth time around the loop you will print Hera again as she is still on the list and the first Greek god.