How to encapsulate a std::set properly?

Question

Hi, I have a class named Particle which has a std::set as a member. The class looks like this:

class Particle {
private:
    std::set<vtkIdType> cells;
    std::set<vtkIdType>::iterator ipc;

public:

    Particle() {};

    enum state {EXISTS = -1, SUCCESS = 0, ERROR = 1};

    state addCell(const vtkIdType cell);

    int numCells() { return static_cast<int>(cells.size()); }

    vtkIdType getFirstCell() { return (*(ipc = this->cells.begin()));}
    vtkIdType getNextCell() { return *(++ipc); }
    vtkIdType hasNextCell() { ++ipc; if (ipc == this->cells.end()) return false; --ipc; return true; }

    std::string getOutput();
};

I'm very unhappy with the getFirstCell(), getNextCell() and especially hasNextCell(), they exist because I don't want to expose the set itself. I had to use the way through ++ipc and --ipc because if((ipc+1) == this->cells.end()) gives a compiler error, ipc+1 seems to be the problem.

What would be a good way to encapsulate a set and access it? Also, is there a nice way to get rid of the getFirstCell() function?

Thanks in advance.

Edit: The code I posted is just an example of the classes structure. The "real" class contains more sets and other data that is not that important for this question (I assumed).

Answer 1

what you show doesn't do anything besides the three getters. encapsulate the set by making the operations that would use these getters part of the Particle class, then you won't need the getters at all: voila, encapsulated.

Answer 2

The reason that ipc+1 does not work is that std::set only supports bidirectional iterators, which support operator++ and operator--; in order to use operator+, you need to use random access iterators.

One concern I see with your design is that your functions are named like accessors (getSuchAndSuch) but they also modify the internal state of the object (ipc is modified). This might lead to confusion.

One thing that you could try would be to use a few member functions that return iterators (a begin and end, for example), and allow users of your class to use iterators to access the internal set, while still encapsulating the set implementation.

You could return the set's iterator type or if you want more control or encapsulation, you could implement your own iterator class that wraps the set's iterator.

Answer 3

I'm not sure why you do not want to expose the set itself, but if it is because you want to ensure that the content of the set cannot be altered outside class Particle just return const iterators which makes the set "read-only", e.g.

typedef std::set<vtkIdType>::const_iterator CellIterator;
CellIterator beginCell() const { return this->cells.begin(); }
CellIterator endCell() const { return this->cells.end(); }

Answer 4

If you'd like to retain the general implementation you already have, but simply eliminate getFirstCell(), you could initialize ipc within the constructor. As stated above, judicious use of const and clear differentiation between accessors and mutators would clarify the interface. Additionally, if you were to implement iterators on your class, then I would recommend that addcell() return an iterator referencing the new cell, and instead throw an exception upon encountering an error.

Answer 5

To prevent exposing set::iterator (to not promise to users more than needed) you can create a wrapper:

class Particle::iterator
{
public:
  iterator()
  {}
  iterator &operator++()
  {
    ++InternalIterator;
    return *this;
  }
  vtkIdType &operator*() const
  {
    return *InternalIterator;
  }
  ...//other functionality required by your iterator's contract in the same way
private:
  iterator(const std::set<vtkIdType> &internalIterator)
    :InternalIterator(internalIterator)
  {}
  std::set<vtkIdType>::iterator InternalIterator;
};

Particle::iterator Particle::GetBeginCell()
{
  return iterator(cells.begin());
}
Particle::iterator Particle::GetEndCell()
{
  return iterator(cells.end());
}

Thus you will get rid of internal iterator (because it's quite restrictive to be able to have only one iterator) and will get ability to use algorithms from STL on Particle's iterators.

Also boost::iterator_facade may be useful here...

Answer 6

The question is really what you're trying to accomplish here. Right now, your class seems (at least to me) to do more harm than good -- it makes working with the contents of the set more difficult instead of easier.

I'd look at Particle, and figure out whether it can provide something meaningful beyond some way of storing/accessing a bunch of cells. If it really is just a simple container, then you'd be a lot better of with something like typedef std::set<cell> Particle;, so the end user can use algorithms and such on this set just like they can any other. I'd only write a class to encapsulate that if you can really encapsulate something meaningful -- i.e. if your Particle class can embody some "knowledge" about particles so other code can work with a particle as a thing that's meaningful in itself.

Right now, your Particle is nothing but a container -- and it doesn't look like a particularly good container either. Unless you can really add something, you might be better off just using what's already there.