Pointer to List Iterator As Function Argument

Hi,

I've written a simple program which involves a class called person, and a list of person objects to represent a list of people. Each person object just has a name and an ID at present. To read stuff from the list of person objects I'm using an iterator.

I figured it would make sense to write a set_iter function that takes a pointer as an argument, so that I could set the iterator to refer to a particular person object with a certain ID, without having to do something like:

Code:

---

iter = personlist.begin()
while (iter != 2)
    iter++;

---

So that's what I've done, by having a buffer iterator in the set_iter function, which effectively does what that code above does whenever you type set_iter(&iter, 2). My first question is:

1. Is the way I've written set_iter an efficient way to do it? Or is there a better way of writing it? Maybe one that doesn't require the buffer iterator to be declared?

Also, a couple of minor questions, if anyone has time:

2. Do you say that the set_iter function has a pointer as an argument, or an address as an argument? Because it has a pointer in the definition but you actually pass an argument.

3. Is a list a good choice of container here or would a vector be better? If so, how come?

Thanks guys.

Code:

---

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

class person
{
private:
    string name;
    int id;
public:
    person(const string& nm, int i) {name = nm; id = i;};
    int getid()        { return id; };
    string getname()    { return name; };
};

list<person> personlist;
list<person>::iterator iter;

void set_iter(list<person>::iterator *target_iter, int target_id);

int main()
{
    personlist.push_back(person("Arnold", 0));
    personlist.push_back(person("Barry", 1));
    personlist.push_back(person("Colin", 2));

    set_iter(&iter, 2);
    cout << "name of person with id 2 = " << iter->getname() << endl;
}

void set_iter(list<person>::iterator *target_iter, int target_id)
{
    list<person>::iterator buffer_iter = *target_iter;
    buffer_iter = personlist.begin();
    while (buffer_iter->getid() != target_id)
        buffer_iter++;
    *target_iter = buffer_iter;
}

---

Why don't you use `std::find'?
Why don't you just return an iterator instead of passing a pointer to an existing one?

Soma

Your usage of globals is not very good. Consider using return values and more meaningful names. (Also fixed some constness error - getter should be const members.)

Code:

---

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

class person
{
private:
    string name;
    int id;
public:
    person(const string& nm, int i) {name = nm; id = i;};
    int getid() const        { return id; };
    string getname() const    { return name; };
};

list<person>::iterator find_by_id(list<person>& persons, int target_id);

int main()
{
    //lets not use globals, OK!?
    list<person> personlist;

    personlist.push_back(person("Arnold", 0));
    personlist.push_back(person("Barry", 1));
    personlist.push_back(person("Colin", 2));

    list<person>::iterator iter = find_by_id(personlist, 2);
    if (iter != personlist.end()) {
        cout << "name of person with id 2 = " << iter->getname() << endl;
    }
    else {
        cout << "person with id 2 not found";
    }
}

list<person>::iterator find_by_id(list<person>& persons, int target_id)
{
    list<person>::iterator result;
    for (result = persons.begin(); result != persons.end(); ++result) {
        if (result->getid() == target_id) {
            break;
        }
    }
    return result;
}

---

However, a preferable way is to use the standard find_if algorithm. Unfortunately the condition is quite complicated, so a function object is needed (matching_id), but you can compare the complexity of that object and the function to find by id specifically.

Another reason why that would be preferable is that for the sake of correctness the previous code would require another overload of find_by_id that takes a const list and returns a const_iterator.

Code:

---

#include <iostream>
#include <list>
#include <string>
#include <algorithm>
using namespace std;

class person
{
private:
    string name;
    int id;
public:
    person(const string& nm, int i) {name = nm; id = i;};
    int getid() const        { return id; };
    string getname() const    { return name; };
};

//function object is something that overloads operator()
class matching_id
{
    int target_id;
public:
    matching_id(int id): target_id(id) {}
    bool operator()(const person& p) const
    {
        return p.getid() == target_id;
    }
};

int main()
{
    //lets not use globals, OK!?
    list<person> personlist;

    personlist.push_back(person("Arnold", 0));
    personlist.push_back(person("Barry", 1));
    personlist.push_back(person("Colin", 2));

    list<person>::iterator iter = find_if(personlist.begin(), personlist.end(), matching_id(2));
    if (iter != personlist.end()) {
        cout << "name of person with id 2 = " << iter->getname() << endl;
    }
    else {
        cout << "person with id 2 not found";
    }
}

---

You don't even need a function object if you can use Boost.Lambda and Boost.Bind, but this is very advanced stuff:

Code:

---

list<person>::iterator iter = find_if(personlist.begin(), personlist.end(), boost::bind(&person::getid, _1) == 2);

---

Quote:

---

3. Is a list a good choice of container here or would a vector be better? If so, how come?

---

It doesn't matter here. The main reason to use list is when you want to insert and removes items everywhere in the container. If you want random access to container contents then vector might be a better choice.

This may be a stupid question, but why should the get functions be const members? According to my C++ book, const is used to ensure you don't modify stuff that shouldn't be modified. But why would the program modify

Code:

---

{ return id; }

---

? Obviously I'm not understanding something here.

Anyway, thanks for your advice guys. That's really useful.

The idea is that you make a getter method look something like this:

Code:

---

class Something {
private:
    int data;
public:
    int get_data() const { return data; }
};

---

That way you're specifying, in effect, that the "this" pointer for get_data() will be const. So you won't be able to modify the class at all (except for any mutable members).

This is good for two reasons. One, this is what you want getters to do. You don't want to let getter methods modify the object. And two, this lets you call the method from a constant object. If you tried to do this, for example,

Code:

---

void print_data(const Something &something) {
    std::cout << something.get_data() << std::endl;
}

---

and get_data() wasn't declared const, you'd get a compiler error.

[edit] I'm assuming tabstop's post below is in response to bengreenwood and not to me . . . ? [/edit]

It wouldn't. That's why it needs to be const, because it won't get modified. You can't call non-const functions on const members -- you should be able to get the value of a const object, so calling a getter makes sense, hence it should be const.

So say if there was another member of person, age, which changed over time. If you had a get_age function, that wouldn't be declared as const would it? Because it might change in future, I mean.

Quote:

---

Originally Posted by bengreenwood

So say if there was another member of person, age, which changed over time. If you had a get_age function, that wouldn't be declared as const would it? Because it might change in future, I mean.

---

You would still declare that member function as const because with respect to that member function, the observable state of the object does not change, even though it may change by the use of other functions.

Quote:

---

Originally Posted by bengreenwood

So say if there was another member of person, age, which changed over time. If you had a get_age function, that wouldn't be declared as const would it? Because it might change in future, I mean.

---

Yes it would still be declared constant, as the getter function shouldn't change anything, just fetch it. A setter function won't be declared as constant as it changes the value passed to it.

EDIT: Laserlight was faster ;)