Using a map of shared_ptr instead of single pointers as class members

Question

I'm working on a node.js bindings for libcurl. As libcurl accepts many callback functions as options (like CURLOPT_READFUNCTION for example), I had to store their respective javascript callbacks somewhere when those options were set.

Initially I had the following class members (Easy.h:71):

Nan::Callback *cbChunkBgn;
Nan::Callback *cbChunkEnd;
Nan::Callback *cbDebug;
Nan::Callback *cbHeader;
Nan::Callback *cbFnMatch;
Nan::Callback *cbOnSocketEvent;
Nan::Callback *cbProgress;
Nan::Callback *cbRead;
Nan::Callback *cbXferinfo;
Nan::Callback *cbWrite;

Initialization also was too much code (Easy.cc:58):

Easy::Easy() :
    cbChunkBgn( nullptr ), cbChunkEnd( nullptr ), cbDebug( nullptr ), cbHeader( nullptr ),
    cbFnMatch( nullptr ), cbOnSocketEvent( nullptr ), cbProgress( nullptr ), cbRead( nullptr ),
    cbXferinfo( nullptr ), cbWrite( nullptr ),
    pollHandle( nullptr ),
    isCbProgressAlreadyAborted( false ), isMonitoringSockets( false ),
    readDataFileDescriptor( -1 ), id( Easy::counter++ ), isInsideMultiHandle( false ), isOpen( true )

Also, since one easy handle can be duplicated, I had a copy constructor which took care of copying the other handle data to itself, this included the callbacks. Most of them looks like that (Easy.cc:120):

if ( orig->cbHeader != nullptr ) {
    this->cbHeader = new Nan::Callback( orig->cbHeader->GetFunction() );
}

Trying to improve this code, I thought on using a map (typedef std::map<CURLoption, std::shared_ptr<Nan::Callback>> CallbacksMap;) which would store all the callbacks being used (new_Easy.h:72).

Copying the callbacks from another handle (in the copy constructor) would look like the following (new_Easy.cc:90):

this->callbacks.insert( orig->callbacks.begin(), orig->callbacks.end() );

Setting/removing a new callback, would looks like the following (new_Easy.cc:1134):

obj->callbacks.erase( CURLOPT_CHUNK_BGN_FUNCTION );
// ...
obj->callbacks[CURLOPT_CHUNK_BGN_FUNCTION].reset( new Nan::Callback( callback ) );

Where obj is an instance of the Easy class.

Retrieving one callback so it can be called (new_Easy.cc:509):

CallbacksMap::iterator it = obj->callbacks.find( CURLOPT_CHUNK_BGN_FUNCTION );
// ...
it->second->Call( ... );

And I would remove all those delete's from the destructor.

Thoughts? Would that be an improvement or just some unnecessary modification? Any better alternatives?

Also I would love any other suggestions for other parts of the code that you feel that are wrong or that can be improved.

Answer 1

I'm not particularly familiar with libcurl, but using a std::map for storing the callbacks is a good thing. One of my rules when programming is; If I need to store more than 1 thing of the same type, use a container for it. Which you've done, and it's a good thing.

There are a couple of nitpicks, and I'm assuming you have the possibility to use C++11.

Variables in class can be initialized with a default value.

...
Nan::Callback *cbChunkBgn = nullptr;
Nan::Callback *cbChunkEnd = nullptr;
Nan::Callback *cbDebug = nullptr;
Nan::Callback *cbHeader = nullptr;
...

Your constructor initializer lists aren't readable.

Change this:

Easy::Easy( Easy *orig ) :
    callbacks(), cbOnSocketEvent( nullptr ), pollHandle( nullptr ),
    isCbProgressAlreadyAborted( false ), isMonitoringSockets( false ),
    readDataFileDescriptor( -1 ), id( Easy::counter++ ), isInsideMultiHandle( false ), isOpen( true )

Into this:

Easy::Easy( Easy *orig ) 
    : callbacks()
    , cbOnSocketEvent( nullptr )
    , pollHandle( nullptr )
    , isCbProgressAlreadyAborted( false )
    , isMonitoringSockets( false )
    , readDataFileDescriptor( -1 )
    , id( Easy::counter++ )
    , isInsideMultiHandle( false )
    , isOpen( true )

You may also benefit from using delegating constructors. I'm not sure how it will fit into your design.

Easy::Easy( Easy *orig ) 
    : Easy()

Maybe you can skip the long constructor initializer lists by using default arguments all together.

Use auto where necessary. Transform this;

CallbacksMap::iterator it = obj->callbacks.find( CURLOPT_FNMATCH_FUNCTION );

Into this;

auto it = obj->callbacks.find( CURLOPT_FNMATCH_FUNCTION );

There is probably more, but those are what I saw at the first glance of the code.