Object pool with linked list of available objects implementation

Design and architecture:

The design of your Object Pool is somewhat unusual. Since you are declaring the payload of type T by value inside the Object struct, once the vector objs is allocated, all objects will also be allocated beforehand. This is fine, if that was your intention, but will require type T to be default constructible.

Another strange thing is the bool create(T newObj); method. You are actually passing a T instance by value. So the pool will in reality only be storing a copy of an existing object. Usually, object/memory pools will keep a set of instances of something that can be recycled. Thus, I would expect the creation method of the pool to return a pointer to an existing (and possibly recycled) object:

T * Pool::create() { /* ... */ }

Possibly returning nullptr or throwing an exception if out of memory or instances to recycle.

The way you deallocate the objects is also unusual. You are again passing a copy of some value of type T to the removeObj() method, which then scans the array of Objects and if finding a match, marks it as unused. Again you are making the assumption that type T can be compared with operator ==, which is not the case for User Defined Types if you don't explicitly implement operator ==. That seems like and unnecessary constraint. In fact, your whole Object Pool seems to be just a thin wrapper over a vector. What I would have expected to see would be a method that returns a pointer to the pool so that is can be recycled. Something in the lines of:

void Pool::free(T * object) { /* ... */ }

I would like to point you to this implementation of a Pool allocator that you can refer to. This was used by the Doom 3 game. It is also based on a linked list, but it uses a list of arrays of objects. Thanks to some pointer manipulation, removal is also constant complexity. Looking at that implementation might make these things I've mentioned more clear to you.

Code review:

Now focusing on the code presented in the question:

1. This is a personal preference of mine, but I like to place the public section of a class first in the header file. My rationale is that people using and reading my code will care much more about the public methods of a class, so that's what they want to see first when reading the header file. Private/protected sections of a class are of interest to the programmer maintaining the code, so it makes sense that they don't need that much visibility.

2. You don't need to implement an empty constructor. Use the new default syntax (C++11):

   // In the declaration inside the class body:
   Pool() = default;
   

3. You should probably make the constructor that takes a single size parameter explicit, to avoid questionable implicit conversions:

   explicit Pool(int size);
   

4. Since your class has some naked pointers as its members, defining its behavior regarding copy and assignment is very important. Read about The Rule of Three. My suggestion at first would be to just make it non-copyable by deleting the assignment operator and copy constructor. It seems like the safest and simplest option in this case:

   Pool(const Pool&) = delete;
   Pool& operator = (const Pool&) = delete;
   

5. The size member variable is unnecessary. std::vector has a member function with the same name: size().

6. Initialize the member vector objs in the constructor initialization list:

   template <class T> Pool<T>::Pool(int size):size(size) 
   {
       objs = std::vector< Object >(size);
       ...
   

   By doing that assignment, you are risking a duplicate initialization. It will first be initialized with the default vector constructor and then again with operator =. I'm not sure if the current generation of compilers is optimizing that or not. I'd guess not because this is a bad programming practice that is easy to fix:

    template <class T> Pool<T>::Pool(int size) 
        : objs(size)
        , numAlive(0)
        , nextAvailable(nullptr)
    {
        ...
    }
   

   Note that you could also use the uniform initialization style if you wanted (e.g. use { } instead of ( ) when calling the constructors of the member variables).

7. Avoid repeating types when declaring objects (remember to DRY):

   std::vector<T*> avl = std::vector<T*>(numAlive);
   

   No need to type std::vector<T*> twice here. You have two options in this case:

   • The "classic" style:

      std::vector<T*> avl(numAlive);
     

   • The new "auto" style from C++11:

      auto avl = std::vector<T*>{numAlive};
     

   I personally like the first "classic" style, however, the second one is starting to be advocated by some programmers, including Herb Sutter.