Is it a good (correct) way to encapsulate a collection?

Question

class MyContainedClass {
};

class MyClass {
public:
  MyContainedClass * getElement() {
    // ...
    std::list<MyContainedClass>::iterator it = ... // retrieve somehow
    return &(*it);
  }
  // other methods
private:
  std::list<MyContainedClass> m_contained;
};

Though msdn says std::list should not perform relocations of elements on deletion or insertion, is it a good and common way to return pointer to a list element?
PS: I know that I can use collection of pointers (and will have to delete elements in destructor), collection of shared pointers (which I don't like), etc.

Answer 1

I think the bigger problem is that you're hiding the type of collection so even if you use a collection that doesn't move elements you may change your mind in the future. Externally that's not visible so I'd say it's not a good idea to do this.

Answer 2

I don't see the use of encapsulating this, but that may be just me. In any case, returning a reference instead of a pointer makes a lot more sense to me.

Answer 3

In a general sort of way, if your "contained class" is truly contained in your "MyClass", then MyClass should not be allowing outsiders to touch its private contents.

So, MyClass should be providing methods to manipulate the contained class objects, not returning pointers to them. So, for example, a method such as "increment the value of the umpteenth contained object", rather than "here is a pointer to the umpteenth contained object, do with it as you wish".

Answer 4

It depends...

It depends on how much encapsulated you want your class to be, and what you want to hide, or show.

The code I see seems ok for me. You're right about the fact the std::list's data and iterators won't be invalidated in case of another data/iterator's modification/deletion.

Now, returning the pointer would hide the fact you're using a std::list as an internal container, and would not let the user to navigate its list. Returning the iterator would let more freedom to navigate this list for the users of the class, but they would "know" they are accessing a STL container.

It's your choice, there, I guess.

Note that if it == std::list<>.end(), then you'll have a problem with this code, but I guess you already know that, and that this is not the subject of this discussion.

Still, there are alternative I summarize below:

Using const will help...

The fact you return a non-const pointer lets the user of you object silently modify any MyContainedClass he/she can get his/her hands on, without telling your object.

Instead or returning a pointer, you could return a const pointer (and suffix your method with const) to stop the user from modifying the data inside the list without using an accessor approved by you (a kind of setElement ?).

  const MyContainedClass * getElement() const {
    // ...
    std::list<MyContainedClass>::const_iterator it = ... // retrieve somehow
    return &(*it);
  }

This will increase somewhat the encapsulation.

What about a reference?

If your method cannot fail (i.e. it always return a valid pointer), then you should consider returning the reference instead of the pointer. Something like:

  const MyContainedClass & getElement() const {
    // ...
    std::list<MyContainedClass>::const_iterator it = ... // retrieve somehow
    return *it;
  }

This has nothing to do with encapsulation, though.. :-p

Using an iterator?

Why not return the iterator instead of the pointer? If for you, navigating the list up and down is ok, then the iterator would be better than the pointer, and is used mostly the same way.

Make the iterator a const_iterator if you want to avoid the user modifying the data.

  std::list<MyContainedClass>::const_iterator getElement() const {
    // ...
    std::list<MyContainedClass>::const_iterator it = ... // retrieve somehow
    return it;
  }

The good side would be that the user would be able to navigate the list. The bad side is that the user would know it is a std::list, so...

Answer 5

Scott Meyers in his book Effective STL: 50 Specific Ways to Improve Your Use of the Standard Template Library says it's just not worth trying to encapsulate your containers since none of them are completely replaceable for another.

Answer 6

std::list will not invalidate any iterators, pointers or references when you add or remove things from the list (apart from any that point the item being removed, obviously), so using a list in this way isn't going to break.

As others have pointed out, you may want not want to be handing out direct access to the private bits of this class. So changing the function to:

  const MyContainedClass * getElement() const {
    // ...
    std::list<MyContainedClass>::const_iterator it = ... // retrieve somehow
    return &(*it);
  }

may be better, or if you always return a valid MyContainedClass object then you could use

    const MyContainedClass& getElement() const {
    // ...
    std::list<MyContainedClass>::const_iterator it = ... // retrieve somehow
    return *it;
  }

to avoid the calling code having to cope with NULL pointers.

Answer 7

@paercebal, @Wilka Thank you for detailed answers. However if I want my ContainedClass to be modified from outside. What way is better (you commonly use):
Writing a set of SetXXXContainedObjectProperty(index, value) methods in MyClass, and returning a const reference or const pointer
or
Return an ordinary pointer (as in original question) and allow changing contained object via public interface.

Answer 8

STL will be more familiar to a future programmer than your custom encapsulation, so you should avoid doing this if you can. There will be edge cases that you havent thought about which will come up later in the app's lifetime, wheras STL is failry well reviewed and documented.

Additionally most containers support somewhat similar operations like begin end push etc. So it should be fairly trivial to change the container type in your code should you change the container. eg vector to deque or map to hash_map etc.

Assuming you still want to do this for a more deeper reason, i would say the correct way to do this is to implement all the methods and iterator classes that list implements. Forward the calls to the member list calls when you need no changes. Modify and forward or do some custom actions where you need to do something special (the reason why you decide to this in the first place)

It would be easier if STl classes where designed to be inherited from but for efficiency sake it was decided not to do so. Google for "inherit from STL classes" for more thoughts on this.

Answer 9

@Shog9: Of course I'm encapsulating MyContainedClass not just for the sake of encapsulation. Let's take more specific example:

class Paragraph {
public:
  // getters and setters for private fields
private:
  FONTFACET m_fontFace;
  FONTSIZET m_fontSize;
  CONTENTT m_content;
}

class Document {
public:
  Paragraph * NewParagraph();
  void DeleteParagraph(Paragraph * p);
  Paragraph * GetParagraph(/*parameters to select particular paragraph*/) {
    std::list<Paragraph>::iterator it = // retrieve according to arguments
    return &(*it);
  }
  // getters and setters of document's properties
private:
  std::list<Paragraph> m_paragraphs;
  // other properties of Document
}

class ToolbarParagraphEditor: public ToolbarWidget {
public:
  // implementation.
private:
  Paragraph * m_paragraph;
}

class DialogParagraphEditor: public DialogWidget {
public:
  // implementation
private:
  Paragraph * m_paragraph;
}

Document obviously owns Paragraphs and Paragraphs don't exist without Document, so encapsulating them in Document seems logical. If we put objects (not pointers) to container they will be destroyed automatically (which is good). There are Editors (View/Controllers according to MVC, or Views according to Doc/View) which can change properties of Paragraph. I think it's reasonable to store pointer to the same paragraph object in both Editors of the same paragraph. So if we will return reference from Document::GetParagraph we will actually return copy built with copy constructor. Considering this example do you commonly use collection of pointers or return a pointer to an element in collection (which doesn't relocate elements) or may be some other method (for ex. write Document::SetParagraphFont(...) and return const reference or pointer)?
UPD: I always used a collection of pointers in such cases. But storing objects in collection in such case will greatly improve the code clarity. Because we can have other objects than use collections of paragraphs, but don't own them. For ex:

class ParagraphSelectionDialog: public DialogWidget {
  // implementation
private:
  std::list<Paragraph *> m_selectedParagraphs; // Selection dialog doesn't own paragraphs
}

And now we can clearly distinguish which classes own other objects and which only use them. What do you think about this?

Answer 10

The Easy way

@cos, For the example you have shown, i would say the easiest way to create this system in C++ would be to not trouble with the reference counting. All you have to do would be to make sure that the program flow first destroys the objects (views) which holds the direct references to the objects (paragraphs) in the collection, before the root Document get destroyed.

The Tough Way

However if you still want to control the lifetimes by reference tracking, you might have to hold references deeper into the hierarchy such that Paragraph objects holds reverse references to the root Document object such that, only when the last paragraph object gets destroyed will the Document object get destructed.

Additionally the paragraph references when used inside the Views class and when passed to other classes, would also have to passed around as reference counted interfaces.

Toughness

This is too much overhead, compared to the simple scheme i listed in the beginning. It avoids all kinds of object counting overheads and more importantly someone who inherits your program does not get trapped in the reference dependency threads traps that criss cross your system.

Alternative Platforms

This kind-of tooling might be easier to perform in a platform that supports and promotes this style of programming like .NET or Java.

You still have to worry about memory

Even with a platform such as this you would still have to ensure your objects get de-referenced in a proper manner. Else outstanding references could eat up your memory in the blink of an eye. So you see, reference counting is not the panacea to good programming practices, though it helps avoid lots of error checks and cleanups, which when applied the whole system considerably eases the programmers task.

Recommendation

That said, coming back to your original question which gave raise to all the reference counting doubts - Is it ok to expose your objects directly from the collection?

Programs cannot exist where all classes / all parts of the program are truly interdependent of each other. No, that would be impossible, as a program is the running manifestation of how your classes / modules interact. The ideal design can only minimize the dependencies and not remove them totally.

So my opinion would be, yes it is not a bad practice to expose the references to the objects from your collection, to other objects that need to work with them, provided you do this in a sane manner

1. Ensure that only a few classes / parts of your program can get such references to ensure minimum interdependency.

2. Ensure that the references / pointers passed are interfaces and not concrete objects so that the interdependency is avoided between concrete classes.

3. Ensure that the references are not further passed along deeper into the program.

4. Ensure that the program logic takes care of destroying the dependent objects, before cleaning up the actual objects that satisfy those references.