Generate a unique numeric identifier for each instance of a class I want to generate a unique ID for every instance of a given class.


The ID is of type unsigned long
When an instance is deleted, the ID is freed (can be reused)


Now, I've come up with the following implementation:

Class Foo 
{
public:

  static unsigned std::set&lt;unsigned long&gt; s_usedID;

  static unsigned long generateID() // Generate a valid ID
  {
    static unsigned long id = 0;

     while (Foo::isIDUsed(id)) // If all ID are taken, create an infinite loop!
        ++id;

     return id;
  }

  static void addID(unsigned long id) // Add a given ID to the set of used ID
  {
    s_usedID.insert(id);
  }

  static void removeID(unsigned long id) // Remove a given ID from the set of used ID
  {
    s_usedID.erase(id);
  }

  static bool isIDUsed(unsigned long id)
  {
    return s_usedID.count(id) == 1 ? true:false;
  }

  explicit Foo() : m_id(Foo::generateID())
  {
    Foo::addID(m_id); // ID is now taken
  }

  virtual ~Foo()
  {
    Foo::removeID(m_id); // Free the ID
  }

private:

  unsigned long m_id;

};


Now for me, this logically works.


Is there a better way to implement this?
Are there any optimizations that I could make?
A few remarks:


The biggest obvious issue is the violation of encapsulation. All methods which allow you to modify the id set are public which means that outside code can change it and break the inherent assumption that each instance will have a unique id.  

The only code which should be able to modify the id set are the constructor and destructor.

If you now say "ah, that won't matter it's just my code and I know not to do this" - programming is a skill and like any skill you get better at it through practice. Which typically means writing code. Therefore you should take any opportunity to practicing writing good code since that helps to form good habits. There is no excuse for being sloppy.
This:


return s_usedID.count(id) == 1 ? true:false



can be written more succinctly as:

return s_usedID.count(id) == 1;

The implementation is obviously not thread-safe. Just something to be aware of.
One way to improve it, is to just use a variable to store the current maximum id and return the next one higher if you need a new one. If an id gets freed up then you could just not worry about it.
If you really want to re-use returned id's then store them in a returnedIds set. The basic logic then becomes:


If returnedIds has any elements pop first element from set and return it
Else new id becomes current_max_id + 1