Thread: Code Check

  1. #1
    Deprecated Dae's Avatar
    Join Date
    Oct 2004
    Location
    Canada
    Posts
    1,034

    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
    Last edited by Dae; 01-07-2009 at 08:40 AM. Reason: virtual dc lol!
    Warning: Have doubt in anything I post.

    GCC 4.5, Boost 1.40, Code::Blocks 8.02, Ubuntu 9.10 010001000110000101100101

  2. #2
    Registered User C_ntua's Avatar
    Join Date
    Jun 2008
    Posts
    1,853
    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&.

  3. #3
    Deprecated Dae's Avatar
    Join Date
    Oct 2004
    Location
    Canada
    Posts
    1,034
    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?
    Warning: Have doubt in anything I post.

    GCC 4.5, Boost 1.40, Code::Blocks 8.02, Ubuntu 9.10 010001000110000101100101

  4. #4
    Registered User C_ntua's Avatar
    Join Date
    Jun 2008
    Posts
    1,853
    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
    Last edited by C_ntua; 01-07-2009 at 10:13 AM.

  5. #5
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    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.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  6. #6
    Deprecated Dae's Avatar
    Join Date
    Oct 2004
    Location
    Canada
    Posts
    1,034
    Just a note guys, most of those choices were intentional (and that code does work fine).

    Quote Originally Posted by iMalc View Post
    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?

    Quote Originally Posted by C_ntua View Post
    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.

    Quote Originally Posted by iMalc View Post
    Perform a search and replace of "this->" with an empty string.
    No thanks, that's intentional.

    Quote Originally Posted by iMalc View Post
    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)();
    Quote Originally Posted by iMalc View Post
    Events.insert(std::make_pair(event, std::vector<VoidFunc>());
    Forgot about make_pair. Thanks iMalc!

    Quote Originally Posted by C_ntua View Post
    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.

    Quote Originally Posted by C_ntua View Post
    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.

    Quote Originally Posted by C_ntua View Post
    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
    Last edited by Dae; 01-07-2009 at 05:03 PM.
    Warning: Have doubt in anything I post.

    GCC 4.5, Boost 1.40, Code::Blocks 8.02, Ubuntu 9.10 010001000110000101100101

  7. #7
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by Dae View Post
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  8. #8
    Registered User C_ntua's Avatar
    Join Date
    Jun 2008
    Posts
    1,853
    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
    Last edited by C_ntua; 01-07-2009 at 06:28 PM.

  9. #9
    Deprecated Dae's Avatar
    Join Date
    Oct 2004
    Location
    Canada
    Posts
    1,034
    Quote Originally Posted by matsp View Post
    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).


    Quote Originally Posted by matsp View Post
    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.

    Quote Originally Posted by matsp View Post
    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).

    Quote Originally Posted by C_ntua View Post
    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.

    Quote Originally Posted by C_ntua View Post
    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
    Warning: Have doubt in anything I post.

    GCC 4.5, Boost 1.40, Code::Blocks 8.02, Ubuntu 9.10 010001000110000101100101

  10. #10
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Location
    Inside my computer
    Posts
    24,654
    Quote Originally Posted by C_ntua View Post
    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.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  11. #11
    Registered User C_ntua's Avatar
    Join Date
    Jun 2008
    Posts
    1,853
    Quote Originally Posted by Elysia View Post
    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.

  12. #12
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by Dae View Post
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  13. #13
    Deprecated Dae's Avatar
    Join Date
    Oct 2004
    Location
    Canada
    Posts
    1,034
    Good to know matsp, thanks a lot.

    Thanks again C_ntua, and Elysia.
    Warning: Have doubt in anything I post.

    GCC 4.5, Boost 1.40, Code::Blocks 8.02, Ubuntu 9.10 010001000110000101100101

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. HELP!!!!emergency Problem~expert please help
    By unknowppl in forum C++ Programming
    Replies: 9
    Last Post: 08-21-2008, 06:41 PM
  2. HELP!!!!emergency ~expert please help
    By unknowppl in forum C Programming
    Replies: 1
    Last Post: 08-19-2008, 07:35 AM
  3. << !! Posting Code? Read this First !! >>
    By kermi3 in forum Game Programming
    Replies: 0
    Last Post: 10-14-2002, 01:27 PM
  4. << !! Posting Code? Read this First !! >>
    By kermi3 in forum C# Programming
    Replies: 0
    Last Post: 10-14-2002, 01:26 PM
  5. Check my code Please
    By AdioKIP in forum C++ Programming
    Replies: 1
    Last Post: 03-12-2002, 08:52 PM