Best practices with object manager

Question

I'm kinda new to c++ programming and I'm coding an object manager to a multiplayer game but I'm having some doubts about how to manage the client objects. The client object is composed of connected client parameters (like IP, connected time, received data etc).

In order to avoid memory fragmentation, I'm planning to allocate a object pool with the max number of client allowed. For that, I'm coding a client object manager like this:

ClientManager.h

#include "Client.h"

class ClientManager { 
public: 
static void init(int max); //initialize pool (allocate memory) 
static void dispose(); //dispose pool (deallocate memory) 
static bool add(int socketFd); //add connected client by its socket file descriptor 
static bool remove(int socketFd); //remove connected client by its socket fd
static Client& get(int socketFd); //get the client object by its socket fd

private: 
Client* clientList; //array of allocated clients objects
int maxClient; //max number of connected clients allowed

Note that this class will be called only the static way so there are no constructors/destructors. This class must be static because it is imperative that the client data can be read/modified between different types of objects.

The implementation will be something like:

ClientManager.cpp

void ClientManager::init(int max) {
maxClient = max;
clientList = new Client[maxClient];
}

void ClientManager::dispose() {
maxClient = 0;
delete [] clientList;
clientList = NULL;
}

bool ClientManager::add(int socketFd) {
//search pool for non-initialized object
//if(there is a non-initializes object) { initialize it with socketFd and return true}
//else return false;
}

bool ClientManager::remove(int socketFd) {
//search pool for socketFd
//if(socketFd found) { clear object (make it non-initialized) and return true}
//else return false
}

Client& ClientManager::get(int socketFd) {
//search for object position
if(pos) return clientList[pos];
else ???????
}

Now, how do I manage the object return in the get function? Is it by reference the best option? I don't want to return a pointer but if it is my last option, I can live with it. I guess I can make sure that I only get registered (initialized) object in the pool, if so is this check in the get function necessary? I don't want an assert because I want the code to be robust and not stop during runtime (I'm new to C++ so if I'm saying something wrong please correct me).

in the main program, I'm thinking in something like:

Daemon.cpp

#include "ClientManager.h"

int main(...) {
ClientManager::init(100);

while(1) {
//do server stuff
//get onConnect event (new client is connecting)
//get onDisconnect event (connected client has gone offline)
//get onDataReceived event (connected client has sent data)
}
}

void onConnect(int socketFd) {
ClientManager::add(socketFd);
}

void onDisconnect(int socketFd) {
ClientManager::remove(socketFd);
}

void onDataReceived(int socketFd) {
do_something(ClientManager::get(socketFd).data);
}

Am I doing it right? Thanks

Notes:
1) This code is on my mind and I typed it here, so I may have forgotten something.
2) The program will terminate only by being killed (I'm using linux), so the ClientManager dispose method will not be explicitly called in the main program (since it is a static class). Again, if I'm saying something wrong please tell me!
3) Sorry about my bad english :)

Answer 1

Couple of comments:

• Use a std::vector to hold your client objects, not some sort of homegrown structure. It'll make your life easier
• I think it's fine if get() returns a reference provided that the user does know that the reference can be invalidated if they hang on to it for too long
• I'd throw an exception in get() if the element isn't there; that's what exceptions are for and it will allow you to handle the missing element at the appropriate level

As to (2), you could handle the appropriate signal and call dispose(). I'd think you probably do want to close the sockets before you exit.

Answer 2

Making lots of the member static doesn't make it a 'static class'. It's just a class like any other, with static member functions. You don't really need to do this anyway: just make ClientManager a normal class and create a single one of them in your game.

I second the advice to use a std::vector instead of your own array. This will make your code more robust and make it easier for you to set up Clients, using the proper constructor for each one instead of using some sort of default constructor for the lot of them.

You shouldn't worry about memory fragmentation - that's such an esoteric low level detail that you shouldn't even be thinking about it. The chance of it being a problem for you is astronomically small. But even if it was to be a problem, we couldn't diagnose it based on what you've posted here, because we don't know what makes up a Client class. There's no point having a carefully managed pool in the ClientManager class if the Client class itself references all sorts of memory elsewhere. At this stage you should concentrate on writing robust and correct code, and optimise later if needed.

You may as well return a pointer from ClientManager::get. Just make sure you check to see if it's null before using it. You can entirely remove the need to return Clients from ClientManager if you choose a different interface, eg. passing the operation do_something along with the data into a member of the ClientManager which will look up the client and call the operation if the client was found, or report an error if it was not. (Although there's no good reason why it would not be found, if your ClientManager is correct.)