CODEWITHSUNDEEP
c++
Michael
Michael

Reputation: 22967

correct use of iterator to return an object from a list

I have a list of meetings:

std::list<meeting*> meetings;

I want to iterate the list and return a reference to a specific meeting:

meeting& day::findMeeting( float StartHour )
{
std::list<meeting*>::iterator it;
for(it = meetings.begin(); it != meetings.end(); it++)
{
    if (it->getStartHour() == StartHour)
    {
        return *it;
    }
}
throw no_such_meeting_error;
}

i get the following errors :

  1. 'getStartHour' : is not a member of 'std::_List_iterator<_Mylist>'
  2. 'return' : cannot convert from 'meeting *' to 'meeting &'
  3. invalid return type 'meeting **' for overloaded 'operator ->'

I'm still learning c++ so would be happy to understand what i'm doing wrong. Also,

Thanks

Upvotes: 2

Views: 3538

Answers (6)

aschepler
aschepler

Reputation: 72431

More a comment than an answer, but too big to put in a comment.

As declared, the function cannot be called given a const day& reference. You might consider fixing that.

If the act of modifying a Meeting is also a modification to the day, overload the function:

class day {
public:
  Meeting& findMeeting(float StartHour);
  const Meeting& findMeeting(float StartHour) const;
  //...
};

The two definitions would look mostly identical, except the second will need const_iterator. Now findMeeting when invoked on a const day& will return a const Meeting&, or if invoked on a day& will still return a modifiable Meeting.

OR -

If modifying a Meeting does not modify the day, then just allow the function to be called either way and always return a modifiable Meeting:

class day {
public:
  Meeting& findMeeting(float StartHour) const;
  //...
};

OR -

If users of class day should not be able to modify its Meetings at all, do the same thing but always return a const Meeting&:

class day {
public:
  const Meeting& findMeeting(float StartHour) const;
  //...
};

Now only class day methods (and friends) are allowed to modify the Meetings which belong to a day. You might also want another private method to find a modifiable meeting, but in this case it should have a different name, not be an overload.

Upvotes: 1

John
John

Reputation: 11

This is not directly related to your question but you should be careful when comparing floats for equality. Floating point numbers are not exact and comparing them with == is not safe.

Have a look at http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm

Upvotes: 1

Eitan T
Eitan T

Reputation: 32930

With all of the above said, I suggest you declare:

std::list<meeting> meetings;

instead of:

std::list<meeting*> meetings;

(correspondingly changing the iterator it to std::list<meeting>::iterator;), and let the std::list container do all the memory-related work for you, unless you specifically want to do it by yourself for some reason.

About returning a reference, the answer depends on your needs. If you want to return the entire object to the calling function, returning the reference is a good idea.

Also, consider using const-reference, const meeting& day::findMeeting(float startHour), if you want the returned object to be read-only, i.e the calling function is able to access the object but not change it.

Upvotes: 3

Flexo - Save the data dump
Flexo - Save the data dump

Reputation: 88751

You have std::list<meeting*>::iterator, but your function promises to return meeting&.

When you say *it that is asking the iterator to give you what it "points to" which in this case will be a meeting* still. You therefore also need to dereference that to get an actual meeting, e.g.:

return **it;

Upvotes: 4

Robᵩ
Robᵩ

Reputation: 168746

I have a list of meetings.

No, you don't. You have a list of pointers to meeting. From that one misunderstanding, all of your further errors flow.

if (it->getStartHour() == StartHour)

This code would be correct if you had a list of meetings. It is wrong if you have a list of pointers to meetings. Try:

if ((*it)->getStartHour() == StartHour)

Next:

return *it;

Try:

return **it;

In the alternative, perhaps you really do want a "list of meetings". In that case, you would declare your list thus:

std::list<meeting> meetings;

I don't know which you want -- a list of meetings or a list of pointers to meetings. That has to do with the design of the rest of your program. I almost never keep a container full of pointers.

You might want a list of pointers, for example, if you need multiple list entries to refer to the same meeting. ("I have a meeting at 10 with Abe, at 11 with Bob and Chuck, and then again a meeting at 10 with Abe"?)

You also might want a list of pointers if copying a meeting is impossible or prohibitively expensive. In that case, I suggest you use a smart pointer rather than a naked pointer.

To answer your other question, yes, returning a reference to an object is a fine thing to do. You do need to be aware of the lifetime of that object; never access the object through its reference after it is destroyed.

Upvotes: 13

Martol1ni
Martol1ni

Reputation: 4702

You have to return **it, when it is a pointer to your object meeting. Returning a reference is fine, but be aware of what you do with it later on. The way pointers work, is that if you edit your reference, the item in the list will also be changed, and vica versa.

Upvotes: 6

Related Questions