This is a post I wrote several years ago and it’s been languishing in my drafts folder ever since. I’m not working on this particular codebase any more. That said, the problems caused by using Java-like getter and setter functions as the sole interface to the object in the context described in the post have a bigger impact these days as they will also affect move construction and move assignment. While I’m not opposed to using getter and setter functions in C++ in general, I am opposed to using them as the only member access method and especially in this particular context where they had to be used to initialise class members that were complex objects themselves.

The old code I was working on at the time -  and it was old, it had survived for over a decade with only minimal changes -  only allowed users to alter the contents of some user-defined data types via getter and setter functions. Even the constructors did not take arguments that allow single step construction of an object. Imagine a class that looks something like this - not the actual implementation, but this is a pretty good approximation:

class MyFoo {
public:
  MyFoo();

  MyFoo clone();

  void Set(complex_object v);
  complex_object Get();

private:
  complex_object val;
}

Also imagine that complex_object follows a similar interface pattern, plus it also contains objects that follow the same pattern again. The code required to create an object of type MyFoo would look something like this:

  MyFoo newFoo;
  newFoo.Set(object_val);

These classes including the contained classes don’t support C++ style copying via assignment operators and copy constructors. Copy constructors and assignment operators weren’t disabled either, but that only added a little icing on the cake when debugging the code. As a result the implementation of copy constructors - actually, ‘clone()’ functions, because someone clearly didn’t trust the built-in C++ mechanisms and decided to reinvent the wheel - was littered with calls like this:

  m_some_member.Set(rhs.m_some_member.Get());

Spotted the not-that-obvious problem ?

For starters, assuming m_some_member is comparatively expensive to construct, the code ends up first default constructing the member and then blatting over its contents and updating it again via the Set() call. Do the double initialisation often enough and you’ll start noticing the performance impact, as does your user.

Also, if you store this kind of object in a standard library container, you still have to copy its contents manually instead of making use of the existing standard library functionality to copy these objects. Storing objects that don’t really follow C++ value semantics in a standard container is generally a really bad idea anyway, but as we all know, no bad idea ever goes unimplemented. Anyay, even on the surface, the fact that the following, comparatively elegant code:

  std::copy(src.begin(), src.end(), dest.begin());

… turns into something really, really ugly like this, which should have given the implementer pause:

  SomeContainerType dest = destinationcontainer.begin();
  for (SomeContainerType::iterator i = somecontainer.begin();
        i != somecontainer.end();
       ++i)
  {
     dest->Set(src->Get());
    ++dest;
  }

Of course there is also the issue that the standard C++ library containers rely on the ability to copy objects held in them due to their value semantics. The containers also don’t know about your special clone functions and accessors. Happy debugging if the compiler-generated copy constructor doesn’t do what you’d expect it to do.

Morale of the story - if you want halfway efficient copy construction, ensure that objects which are contained in other objects properly support and if necessary, implement copy constructors and assignment operators. If you must write code like the above, at least don’t put the objects into standard library containers that expect value semantics. To prevent someone from putting them into standard library containers, you can either use my preferred solution of inheriting from something like boost::noncopyable or declare both the copy constructor and assignment operators private.

Part of the problem described above is that the code was mixing multiple incompatible idioms here, which is something that rarely works well. That is never a good idea unless your idea of fun is debugging your way through the night.

So, while functions that access internal members of an object are generally a good idea as part of encapsulation, please make sure that getter and setter functions aren’t the only possible way to access internal objects. Don’t make your life harder by encapsulating implementation details away from yourself to the extent that you end up with a very big gun pointed at your foot.