C++ using vector iterator correctly

Question

I'm new to C++ and I have a vector of doctors.
I add a new doctor with the following code:

void DoctorAdmin::setDoctor(std::string lastname, std::string forename,
 Person::Sex sex){

    //Create new doctor
    Doctor* doc = new Doctor(lastname, forename, sex);

    //insert at the end of the vector
    doctors.push_back(doc);
}

Then I want to show their information on the console:

void DoctorAdmin::showDoctors(){

cout << "Doctors:" << endl;
cout << "Name" << "\t\t\t" << "Forename" << "\t\t\t" << "Sex" << endl;

for (vector<Doctor*>::iterator i = doctors.begin(); i != doctors.end(); i++){

    Doctors* doc = doctors.at(i);
    cout << doc->getName() << "\t\t\t" << doc->getForename() << "\t\t\t" 
         << doc->getSex() << endl;
}

After doing it like this I get two Errors:

E0304   No instance of overloaded function "std::vector<_Ty, _Alloc>::at [mit _Ty=Doctors *, _Alloc=std::allocator<Doctors *>]" matches the argument list.

// and

C2664   "Doctors *const &std::vector<Doctors *,std::allocator<_Ty>>::at(const unsigned int) const" : cannot convert from Argument "std::_Vector_iterator<std::_Vector_val<std::_Simple_types<_Ty>>>" in "const unsigned int" 

How do I use the vector iterator correctly to avoid this?

Answer 1

The at function requires an index, but a vector<Arzt*>::iterator is not an index, neither semantically nor technically. An iterator points directly to an element, whereas an index represents the distance between a container's start and the element in a container that allows random element access.

Because an iterator points directly to an element, the at function isn't even necessary in your loop. *i yields the element:

Arzt* doc = *i;

Beginning with C++11, the code for such simple loops can be written in a shorter way using auto:

for (auto i = aerzte.begin(); i != aerzte.end(); i++){

The compiler knows what type i really is because it knows what begin() returns.

Even better, use a range-based loop:

for (auto doc : aerzte){
    cout << doc->getName() << "\t\t\t" << doc->getVorname() << "\t\t\t" 
         << doc->getGeschlecht() << endl;
}

And while we're at it, don't use dynamic memory allocation when you don't have to. This isn't Java or C#; new is dangerous territory in C++ and should be avoided:

#include <vector>
#include <string>
#include <iostream>

struct Arzt
{
    Arzt(std::string const& name, std::string const& vorname) :
        name(name),
        vorname(vorname)
    {
    }

    std::string name;
    std::string vorname;
    // Geschlecht omitted for brevity's sake
};

int main()
{
    std::vector<Arzt> aerzte;

    Arzt doc1("foo", "bar");
    Arzt doc2("foo", "bar");
    Arzt doc3("foo", "bar");

    aerzte.push_back(doc1);
    aerzte.push_back(doc2);
    aerzte.push_back(doc3);

    for (auto const& arzt : aerzte)
    {
        std::cout << arzt.name << ' ' << arzt.vorname << '\n';
    }
}

As you are no longer iterating over pointers but over larger objects, const& should be used in the for loop.

Answer 2

An iterator is not index-like, it is pointer-like.

for (vector<Arzt*>::iterator doc = aerzte.begin(); doc != aerzte.end(); doc++)
{
    cout << (*doc)->getName() << "\t\t\t" << (*doc)->getVorname() << "\t\t\t" 
         << (*doc)->getGeschlecht() << endl;
}

It seems like you are confused as to when you need to new things too. Most of the time you don't need new

vector<Arzt> aerzte;

void ArztAdmin::anlegenArzt(std::string name, std::string vorname, Person::Geschlecht geschlecht){
    // Create new doctor at the end of the vector
    aerzte.emplace_back(name, vorname, geschlecht);   
}

You can also directly bind references as loop variables

for (Arzt & doc : aerzte)
{
    cout << doc.getName() << "\t\t\t" << doc.getVorname() << "\t\t\t" 
         << doc.getGeschlecht() << endl;
}