0

I have the two classes Player and Team, where each player class has a pointer to the team

class Player{
private: Team* team;}

and each team has a list of pointers to the players

class Team{
private: list<Player*> playerlist;}

Now i want to find out in which team a player plays by adressing him in the list. Afterwards i want to be able to adress the team for something like team.getid()

I tried the following solution for a function:

std::list<Team>::iterator* teamsearch(std::list<Team> a, int b)
{
    int i = 0;
    std::list<Team>::iterator Listitem;
    std::list<Team>::iterator* Listitemtest = &Listitem;
    while (i == 0){
        for (Listitem = a.begin(); Listitem != a.end(); ++Listitem)
        {
            if (Listitem->getteamID_() == b)
                i++;
            Listitemtest = &Listitem;
        }

    };
    std::cout << "TeamID: " << (*Listitemtest)->getteamID_() << std::endl;
    return Listitemtest;
}

The idea is that it returns an iterator which points to the adress of the team of the player. Is this correct ?

Then i tried to adress the teams by dereferencing the iterator again by this way:

Team team;
team.setteamID_(0);
team.setteamname_("team1");
Teamvector.push_back(team);
std::list<Team>::iterator Listitem;
Listitem = *teamsearch(Teamvector, 0);

which gives me a runtime error in MSVC120.dll ...include\list: list iterator not derefencable

Somehow it seems like the iterator points to the end of the teamlist ?

jimbo999
  • 80
  • 7
  • you're assigning unconditionally to `Listitemtest`. also, the `while` loop seems to be meaningless but can cause infinite looping when list item not found. – Cheers and hth. - Alf Mar 13 '14 at 23:20
  • What do u mean by your first sentence? i wanted to have some condition to stop the if loop when the team id is found and have read that "break" is not a good idea to use. – jimbo999 Mar 13 '14 at 23:22
  • Wouldn't it be better to have an `std::vector` inside each team? – OMGtechy Mar 13 '14 at 23:22
  • Why don't you return the iterator, instead of a pointer to it? – Jonathan Wakely Mar 13 '14 at 23:24
  • I had this first, but what if i have to add a player? then the pointers will be point to the wrong adress when it is a vector. And working withouth pointers seems not a good idea to me here. @Jonathan I had this before without a pointer, it ran into the same problem so i thought this could help – jimbo999 Mar 13 '14 at 23:25
  • 1
    @jimbo999, working with pointers is almost never a good idea. – Shoe Mar 13 '14 at 23:26
  • @jimbo999 Why would the pointers point to the wrong address? – Etienne de Martel Mar 13 '14 at 23:27
  • 4
    Wielding pointers does not bring the rains. http://en.wikipedia.org/wiki/Cargo_cult_programming and http://programmers.stackexchange.com/questions/201972/how-to-overcome-programming-by-coincidence – sehe Mar 13 '14 at 23:27
  • @ Etienne de Mertel: Adding elements to a vector do change the addresses where the elements are stored, right ? So if a Team has pointers to a player inside the playervector and i add a player in this playervector this pointer does not address the original player anymore. Or am i wrong ? – jimbo999 Mar 13 '14 at 23:33
  • @jimbo999 Yes - depending. See [Iterator invalidation Rules](http://stackoverflow.com/questions/6438086/iterator-invalidation-rules/6438087#6438087). Also, use `std::list` for stable iterators, `boost::stable_vector` or just pass indices around – sehe Mar 13 '14 at 23:39

2 Answers2

4

You are returning a pointer to a local variable. The iterator which is pointed to by the returned pointer, is already destroyed when the function returns.

You should just return by value, don't bother with pointers here. An iterator is trivial to copy.

Oktalist
  • 14,336
  • 3
  • 43
  • 63
  • And what could i do to solve this problem ? Thinking about all this pointers makes me crazy sometimes – jimbo999 Mar 13 '14 at 23:27
  • So stop using pointers, return the iterator directly instead of fscking about taking its address and using pointers to objects that don't exist any more – Jonathan Wakely Mar 13 '14 at 23:30
  • Okay, so i should work without pointers completely? Even if the classes and their objects all are related somehow ? I thought this was some ideal case to use pointers... – jimbo999 Mar 13 '14 at 23:37
1

Nobody mentioned this yet but the first major problem is that this line:

Listitemtest = &Listitem;

means that Listitemtest points to the iterator Listitem itself. It does not point to the thing that ListItem is pointing at.

Therefore when you go (*Listitemtest)->getteamID_(), this is equivalent to Listitem->getteamID_(). But since Listitem is now at a.end() this is an invalid dereference.

To fix this, you should stop using pointers to iterators. I can't think of a single situation ever where it is a good idea to use pointers to iterators - instead it's a sign that you're doing something wrong.

Instead, make Listitemtest be an iterator of the same type as Listitem, and then just set it by going Listitemtest = Listitem. Then it will point that the item you are interested in.

However, having fixed this, you still can't return Listitemtest yet. That is because it points into std::list<Team> a, which is a local variable to your function and will stop existing when you return. To fix this, make the parameter be std::list<Team> &a. Then you can safely return the iterator.

M.M
  • 138,810
  • 21
  • 208
  • 365
  • NB. If your main way of looking up teams is by their ID, you may want to consider using a `std::map` instead of a `list`. Then you don't even need this function at all, you just call `my_map.find(id)`. – M.M Mar 14 '14 at 00:02
  • Thx i did this already and it fixed this error but still i'm not able to deference the iterator given back by the function. std::cout << Listitem->getteamID_() does work the way it should in the function but after returning the iterator this gives the dereference error again. Thx for the map idea will think about it – jimbo999 Mar 14 '14 at 00:02
  • I just updated my post as I left off an important detail – M.M Mar 14 '14 at 00:03
  • Also, as the other guys said, you need to handle the case where the team is not found at all -- either throw an exception; or return `a.end()` and have the calling function check it didn't get back `a.end()` before dereferencing. – M.M Mar 14 '14 at 00:04
  • Thx i implemented that but that was not the error reason as i said, in the function ListIterator dereferencing works well, after returning it it doesnt anymore – jimbo999 Mar 14 '14 at 00:12
  • If it's still not working post the actual code that is not working now – M.M Mar 14 '14 at 00:30