Accessing class member of std:vector of std:vector in a class with another std:vector

Question

I am puzzled by how to access a std::vector in a class with another a std::vector. A previous discussion(Why is it OK to return a 'vector' from a function?) deals with a similar issue, but it seems I must have misunderstood something.

Here is a sketch of my code:

class Employee
    m_id;
public:
    set_id(int id);
    id();

In Employee.cpp
void Employee::set_id(const int id)
{
    m_id = id;          // m_id is 5
}

int Employee::id() const
{
    return m_id;        // m_id is rubbish
}

class Company
public:
    std::vector<Employee> m_employees;

In Company.cpp
void Company::addEmployee(const addEmployee employee) // A vant to pass a copy of employee
{
    m_employees.push_back(employee);
}
std::vector<Employee> Company::employees()
{
    return m_employees;
}

in myClass.h
std::vector<Company> m_companies;

in myClass.cpp  

int empl_id = 5;   // test value
m_companyIndex = 0;   // for test
m_emplIndex = 0;   // 

Company company; 
Employee employee;
m_companies.push_back(company);
m_companies.at(m_companyIndex).addEmployee(employee);

            m_companies.at(m_companyIndex).employees().at(m_emplIndex).set_id(empl_id);
int idRet = m_companies.at(m_companyIndex).employees().at(m_emplIndex).id(); 

idRet contans an apparently random number, instead of 5. Using a debugger shos that 

If I instantiate Employee in myClass I can set and read back m_id; idem for Company, so the problem seems to be realted to std::vector m_employees; in class Company. Could it have something to do with "Return Value Optimisation"? By the way, I am using gcc 7.5.4.

Answer 1

As @dxiv points out, your Company::employees function constructs and returns a copy of the m_employees list. Thus your code

m_companies.at(m_companyIndex).employees().at(m_emplIndex).set_id(empl_id);

translates to:

Get the element of m_companies at the index m_companyIndex,
then construct a copy of its m_employees list,
then get the element of that copy at index m_emplIndex,
then set the id of that element to empl_id,
and then destroy the copy.

This of course does not change the contents of the m_employees list at all.

You could make your code work more or less correctly by making Company::employees return a non-const reference to the employees list, rather than a copy. However, such a function could not be called on a 'const' Company object, so you would probably also want a 'const' version of it that returned a const reference as well.

However, exposing a class's private implementation details, such as m_employees, even via a non-const reference, is dangerous, because if outside code uses this reference to modify the m_employees vector, then the Employees class is not notified, and any other internal data that it has which may depend on the contents of the m_employees vector can't be updated at the same time.

It is therefore generally better for outside code to ask the containing class (Employees) to make the change rather than asking the containing class to return a non-const reference that can be used by outside code to make the change. That way, the Employees class can make the change and also update any other internal data that it has that may depend on the m_employees contents.

For instance, you could add an Employees::at function which returns a reference to an element so that the above line of code could be rewritten as:

m_companies.at(m_companyIndex).at(m_emplIndex).set_id(empl_id);

or even better:

m_companies.at(m_companyIndex).set_id(m_emplIndex, empl_id);

or even better, if m_companies was contained in a Companies object named 'companies':

companies.set_id(m_companyIndex, m_emplIndex, empl_id);

With these functions, you could change Employees::m_employees to use a completely different data structure (say, a sorted list, linked list or set) without affecting your interface at all. This reduces the coupling between outside code and the internals of your class, and makes things easier to change in the future if you need to.

In general, it's wise to consider the Law of Demeter when designing interfaces of this sort, especially with regard to interfaces for mutating the contents of objects: Many objects don't react well to having their internal contents changed by outside classes without their knowledge, so it's best to ask the containing object to change its subobjects when possible, rather than allowing non-const references to them to be accessible from outside the class.