English 中文(简体)
How to encapsulate a std::set properly?
原标题:

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).

最佳回答

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(); }
问题回答

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.

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...

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.

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.

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.





相关问题
Undefined reference

I m getting this linker error. I know a way around it, but it s bugging me because another part of the project s linking fine and it s designed almost identically. First, I have namespace LCD. Then I ...

C++ Equivalent of Tidy

Is there an equivalent to tidy for HTML code for C++? I have searched on the internet, but I find nothing but C++ wrappers for tidy, etc... I think the keyword tidy is what has me hung up. I am ...

Template Classes in C++ ... a required skill set?

I m new to C++ and am wondering how much time I should invest in learning how to implement template classes. Are they widely used in industry, or is this something I should move through quickly?

Print possible strings created from a Number

Given a 10 digit Telephone Number, we have to print all possible strings created from that. The mapping of the numbers is the one as exactly on a phone s keypad. i.e. for 1,0-> No Letter for 2->...

typedef ing STL wstring

Why is it when i do the following i get errors when relating to with wchar_t? namespace Foo { typedef std::wstring String; } Now i declare all my strings as Foo::String through out the program, ...

C# Marshal / Pinvoke CBitmap?

I cannot figure out how to marshal a C++ CBitmap to a C# Bitmap or Image class. My import looks like this: [DllImport(@"test.dll", CharSet = CharSet.Unicode)] public static extern IntPtr ...

Window iconification status via Xlib

Is it possible to check with the means of pure X11/Xlib only whether the given window is iconified/minimized, and, if it is, how?

热门标签