Code Check

It's been a few years since I did anything with C++. I'm going to read and review some books. In the meantime I need to know if there's any apparent problems with this code, Meaning stack vs heap allocation, function pointer, stl map/vector/pair, etc. For instance I couldn't get associative arrays working so I quickly threw in "->find(event)->second->" instead of "[event]->", and "this->Events->insert" instead of "this->Events[event] =".

Please don't mind my syntax.

Yeah....... o.o

Code:

#include <map>
#include <vector>
#include <string>
#include <iostream>
#include <utility>

class EventDispatcher
{
    public: std::map<std::string, std::vector<void (*)()>* >* Events;

    public: EventDispatcher();
    public: virtual ~EventDispatcher() {}

    public: void AddEvent(const char*);
    public: void RemoveEvent(const char*);
    public: void DispatchEvent(const char*);

    public: void AddEventListener(const char*, void (*)());
    public: void RemoveEventListener(const char*, void (*)());

    public: bool EventExists(const char*);

    public: void Clear();
    public: bool Empty();
};

void callback()
{
    std::cout << "Called" << std::endl;
}

int main()
{
    EventDispatcher* eventDispatcher = new EventDispatcher();

    eventDispatcher->AddEvent("a");
    eventDispatcher->AddEvent("b");
    eventDispatcher->AddEvent("c");

    eventDispatcher->AddEventListener("b", &callback);

    //eventDispatcher->RemoveEventListener("b", &callback);

    eventDispatcher->DispatchEvent("b");

    std::cin.get();
}

EventDispatcher::EventDispatcher()
{
    this->Events = new std::map<std::string, std::vector<void (*)()>* >();
}

void EventDispatcher::AddEvent(const char* event)
{
    this->Events->insert(std::pair<std::string, std::vector<void (*)()>* >(std::string(event), new std::vector<void (*)()>()));
}

void EventDispatcher::RemoveEvent(const char* event)
{
    if(this->EventExists(event))
        this->Events->erase(event);
}

bool EventDispatcher::Empty()
{
    return this->Events->empty();
}

void EventDispatcher::Clear()
{
    if(!this->Empty())
        this->Events->clear();
}

/**
* Usage: eventDispatcher.AddEventListener("eventName", &func);
*/
void EventDispatcher::AddEventListener(const char* event, void (*listener)())
{
    if(!this->EventExists(event))
        this->AddEvent(event);

    this->Events->find(event)->second->push_back(listener);
}

bool EventDispatcher::EventExists(const char* event)
{
    return this->Events->find(event) != this->Events->end();
}

void EventDispatcher::RemoveEventListener(const char* event, void (*listener)())
{
    if(this->EventExists(event))
        for(std::vector<void(*)()>::iterator i = this->Events->find(event)->second->begin(), l = this->Events->find(event)->second->end(); i != l; i++)
            if((*i) == listener)
                this->Events->find(event)->second->erase(i);
}

void EventDispatcher::DispatchEvent(const char* event)
{
    for(std::vector<void(*)()>::iterator i = this->Events->find(event)->second->begin(), l = this->Events->find(event)->second->end(); i != l; i++)
        (*(*i))();
}

Thanks in advance

Code:

void EventDispatcher::AddEvent(const char* event)
{
    this->Events[std::string(event)] = new std::vector<void (*)()>;
}

void EventDispatcher::AddEventListener(const char* event, void (*listener)())
{
    if(!this->EventExists(event))
        this->AddEvent(event);

    this->Events[std::string(event)].push_back(listener);
}

I believe these should work. Haven't ever used a map so don't take my word on it.
It also seems that since you have a map of <string, vector<...>> you might want to pass event as a string rather than a const char*. Just an idea. Probably the best is to pass const std::string&.

I tried that, minus the char array to string conversion. Either way no luck. Which is weird since I checked the reference and it looked fine. The only difference is I'm allocating the map/vector on heap - which complicates it a bit.

Forgot about const string&, thanks C_ntua. Still no luck though, haha

Anything else?

Use

Code:

public:
   ...
   ...

rather than

Code:

public :
public :
....

No need to add a lot of public. But that is style just noting an alternative in case you didn't know

Well, it doesn't really matter where you allocate the map. The operator [] works like a combined insert/find. If it finds it then you just assign the value. If it doesn't find the key it inserts a new one (linke). But again, I am really a newb with std::map

On other note, why do you have it on the heap? You can have it on the stack and also use a destructor. That is clearly better. Otherwise you need to delete everything you newed. The EventDispatcher, the elements added to your map.
This is what I propose:

Code:

public: virtual ~EventDispatcher() {
     Events->clear();
}

//main
EventDispatcher eventDispatcher;

Note that an EventDispatcher doesn't really take a lot of memory. It will be a few bytes. The map can be huge, so that should be dynamically allocate. But the elements of map, vector etc are always dynamically allocated. So you don't need to dynamically allocate anything at all.
By the way you have this:

Code:

public: std::map<std::string, std::vector<void (*)()>* >* Events;

So you have a pointer to a map. Do you allocate memory for it??? (If you don't I am suprised the program doesn't crash).I don't see anything like Events = new std::map. Maybe I just can't spot it If you don't have it put it in the constructor.

In any case, you don't need that. Just do:

Code:

public: std::map<std::string, std::vector<void (*)()>* > Events;

So you statically allocate a map. The elements of map, as noted above, will be dynamically allocated. That is the insert() function job. And note that they are deleted with clear().

EDIT: I am not sure about my destructor comment. I believe if you use Events as a map not a pointer to a map, the destructor of map should take care of the deletion of its elements.
EDIT2: Err, sorry you allocate memory for Events. Believe me sometimes I can really get blind For me "the best way to find something is to stop looking for it". So you can guess how good I am on spotting things.
But again, that is not needed. Declare it normally and don't use the constructor.
Generally have this in mind:
1) Pointer, references can be allocated on the stack
2) Objects allocated on the heap if they are large
A map, a vector, a list etc are just pointers. They just point to a memory chunk. When you insert an element that is inserted dynamically.
If you replace "Events->" with "Events." and delete the constructor and emmit the * in the map declaration, you have what you want

Create a typedef for your function pointer:

Code:

typedef void(*)() VoidFunc;

Change your declaration of Events to:
std::map< std::string, std::vector<VoidFunc> > Events;

Perform a search and replace of "this->" with an empty string.
Perform a search and replace of "Events->" with "Events.".

Empty out your constructor:

Code:

EventDispatcher::EventDispatcher()
{
}

Fix AddEvent:

Code:

void EventDispatcher::AddEvent(const std::string* event)
{
    Events.insert(std::make_pair(event, std::vector<VoidFunc>());
}

Plus lots more along the same lines.

Just a note guys, most of those choices were intentional (and that code does work fine).

Originally Posted by iMalc

Change your declaration of Events to:
std::map< std::string, std::vector<VoidFunc> > Events;

I'd like a more convincing argument why the map has to be allocated on the stack rather than dynamically on the heap. Can anyone here point out the differences rather than following what they found in a tutorial or book?

Originally Posted by C_ntua

On other note, why do you have it on the heap? You can have it on the stack and also use a destructor. That is clearly better. Otherwise you need to delete everything you newed. The EventDispatcher, the elements added to your map.

The elements added to the map will have to be deleted if they are newed even if the map isn't newed. The only difference I know of is one line, and the different advantages between stack and heap which no one has mentioned yet.

Originally Posted by iMalc

Perform a search and replace of "this->" with an empty string.

No thanks, that's intentional.

Originally Posted by iMalc

Create a typedef for your function pointer

Good point. I guess I could typedef it. Would make things easier That syntax doesn't work for function pointers by the way. This does:

Code:

typedef void (*EventFunction)();

Originally Posted by iMalc

Events.insert(std::make_pair(event, std::vector<VoidFunc>());

Forgot about make_pair. Thanks iMalc!

Originally Posted by C_ntua

Use
...
No need to add a lot of public. But that is style just noting an alternative in case you didn't know

I know :P That's intentional. It's my personal syntax choice, and it has no overhead. Notice besides the colon it looks the same as most languages? Java, C#, ActionScript, etc.

Originally Posted by C_ntua

Well, it doesn't really matter where you allocate the map. The operator [] works like a combined insert/find. If it finds it then you just assign the value. If it doesn't find the key it inserts a new one (linke). But again, I am really a newb with std::map

Right, but as you can see the [] operator isn't working.. yet.

Originally Posted by C_ntua

A map, a vector, a list etc are just pointers. They just point to a memory chunk. When you insert an element that is inserted dynamically.
If you replace "Events->" with "Events." and delete the constructor and emmit the * in the map declaration, you have what you want

Okay, so vectors, maps, etc. dynamically allocate (will use pointers) -internally- no matter how you allocate them. So then wouldn't there be little to no difference allocating the one variable on the heap instead (small difference)? Other than syntax difference (arrow vs dot access), and the ability to change the pointer? So there's no problem with doing it this way?

That's a few improvements already. Thanks guys

Originally Posted by Dae

I'd like a more convincing argument why the map has to be allocated on the stack rather than dynamically on the heap. Can anyone here point out the differences rather than following what they found in a tutorial or book?

Unless you have a good reason why it NEEDS to be allocated on the heap, the norm should be to not allocate from the heap.

Since it is a class member, it is not (as such) allocated on the stack - it is allocated from whatever memory that the class itself is allocated from - and that depends on how the class is instantiated - if you use new to create a class object, then it comes from the heap,
if your eventclass is created on the stack, then your Events member variable will use space on the stack. If it's a global variable (no, we do not suggest this), then it's allocated from global data space.

The only advantage with having a pointer instead of instantiating the Events map object in the class would be if you have lots of EventDispatcher instances (e.g. a large array of them), where a lot of these are not actually used, and you do not allocate the map object in the constructor, but at some later stage - then you would save some memory because of only using 4 or 8 bytes for the pointer, rather than the space needed for a std::map object.

I suspect in this case, you are not going to have a large number of EventDispatcher instances, and you are currently allocating in the constructor, so you do not have any benefit - you are just using more memory space, since you are taking up 4 bytes for the pointer, and then the space for a std::map object. Also the memory allocation takes up more space than the actual object, since it has a "admin block" of something like 16-64 bytes, and another few bytes because the size of an allocation is normally rounded to some nice even 2^n bytes, at least 8, often 16 or 32 bytes - so you probably use 64 bytes of heap memory. I don't know how large a std::map object is, but probably around 16-32 bytes (obviously, as the map object is filled with data, it will in itself use heap storage for that, but that's regardless of how you are storing the actual map object).

--
Mats

As for the deletion. The elements will have to be deleted in std::map, but that can be automatically with a destructor. Which won't work if you dynamically allocate memory on the heap (note that std::map has a destructor by its own).
So if you have this:

Code:

map<..>* m = new man<..>();

then you need to do this:

Code:

delete m;

in order to free the map pointer AND call map's destructor.
So in your code you would need to add a delete statement in your EventDispatcher's class destructor to delete the map you dynamically allocate. Every new needs a delete

If the EventDispatcher is also dynamically allocate then you need to have delete in your destructor and you need to call delete in the main program.

So statically allocated objects offer the functionality to call the destructors automatically when they go out of scope and you don't have to worry about them.

And its faster, I believe. Not sure and don't know why, but that is what people usually do

As for the [] I ll try them and tell you if I figure out why. But you don't need to use them. You ll gain nothing, since you already wrap anything around functions. But I am just curious

EDIT: Replacing Events->find(event)->second with (*Events)[std::string(event)] works fine. I suspect you forgot the * for Event. If you forget it you will have an error because you are using the [] for a pointer (Events) not a map (*Events, where the Events pointer points). But that doesn't matter really. Insert should be faster since it doesn't search and insert as it does with []. For find it should be the same speed

EDIT2: Also, note that new needs some time to execute and delete. So it is a bit slower dynamically allocating than statically allocating

Originally Posted by matsp

The only advantage with having a pointer instead of instantiating the Events map object in the class would be if you have lots of EventDispatcher instances (e.g. a large array of them), where a lot of these are not actually used, and you do not allocate the map object in the constructor, but at some later stage - then you would save some memory because of only using 4 or 8 bytes for the pointer, rather than the space needed for a std::map object.

Oh, right, that's it. If I'm allocating the map later then it's an advantage. Since I'm allocating it in the constructor I haven't done anything except increase memory consumption (via pointer). Although since I'm not allocating many, that really isn't an issue, and if I am allocating many then I may want to do it dynamically (funny).

Originally Posted by matsp

Unless you have a good reason why it NEEDS to be allocated on the heap, the norm should be to not allocate from the heap.

Alright, I'll use the norm. I just wanted a reason, other than "because this is how it's always done" and I always read about stack overflows. Plus I just like the consistent arrow syntax.

Originally Posted by matsp

If it's a global variable (no, we do not suggest this), then it's allocated from global data space.

Thanks, I'm not THAT new. I avoid globals at all costs (hey, look, static methods, lol), even though there are some appropriate uses (extern and especially in C).

Originally Posted by C_ntua

As for the deletion. The elements will have to be deleted in std::map, but that can be automatically with a destructor. Which won't work if you dynamically allocate memory on the heap (note that std::map has a destructor by its own).
So if you have this:

Code:

map<..>* m = new man<..>();

then you need to do this:

Code:

delete m;

in order to free the map pointer AND call map's destructor.
So in your code you would need to add a delete statement in your EventDispatcher's class destructor to delete the map you dynamically allocate. Every new needs a delete

If the EventDispatcher is also dynamically allocate then you need to have delete in your destructor and you need to call delete in the main program.

Exactly. I already had that in my updated code. You said because I was dynamically allocating the map variable I had to call delete on every element of the map though. Which isn't true, or at least isn't worded correctly.

Originally Posted by C_ntua

EDIT: Replacing Events->find(event)->second with (*Events)[std::string(event)] works fine. I suspect you forgot the * for Event. If you forget it you will have an error because you are using the [] for a pointer (Events) not a map (*Events, where the Events pointer points). But that doesn't matter really. Insert should be faster since it doesn't search and insert as it does with []. For find it should be the same speed

Oh yeah! It had to be dereferenced. Wow, forgot, since it's a pointer. There's another reason to declare it the normal way. Thanks again

Originally Posted by C_ntua

And its faster, I believe. Not sure and don't know why, but that is what people usually do

Well, dynamic memory has its own overhead when creating them. Apart from that, there is no extra overhead.

Insert should be faster since it doesn't search and insert as it does with []. For find it should be the same speed

I believe it does search. It needs to find the correct position to insert it. Well, perhaps it will not search as much as operator [], but the speed difference should be negligible.

Originally Posted by Elysia

I believe it does search. It needs to find the correct position to insert it. Well, perhaps it will not search as much as operator [], but the speed difference should be negligible.

You are absolutely right. Besides in map the keys are unique so a search has to be done anyways.

Originally Posted by Dae

Alright, I'll use the norm. I just wanted a reason, other than "because this is how it's always done" and I always read about stack overflows. Plus I just like the consistent arrow syntax.

Yes, stack overflow is certainly something to consider - however, I'm SURE that a map object is only a few dozen bytes or so - so as long as you do not have LOTS of EventDispatcher objects created on the stack, it won't overflow the stack unless you have some other huge data items on the stack already [and if so, then you should consider making those allocated on the heap instead].

There is extra overhead in using pointers - both when accessing them, and when creating and destroying them, as the C++ library will have to be called to allocate and free the memory - compared to stack-based allocation, that will be 100-1000x longer. Not a big deal if you do it rarely. And every pointer access is at least one, often two, more operations to get to the actual data item. On it's own, that's no big deal - but do it all over the place, and it will add up. So I'm saying this more because I think you should understand what the consequences are, rather than because I think it will cause a big problem with this code - I think the most work will be in the actual handler called by the dispatcher, rather than the dispatcher itself.

--
Mats

Good to know matsp, thanks a lot.

Thanks again C_ntua, and Elysia.