Skip to main content
Commonmark migration
Source Link

###Inheritance is by default private

Inheritance is by default private

class concreteObserver : ObserverInterface 
                        ^^^^ Need public: here

Mark overriden methods:

void update(std::string message) // override ???

endl vs '\n'

Don't use endl unless you absolutely want to flush the buffer. Probably OK here. Just wanted to note it.

###Don't pass pointers:

Don't pass pointers:

    void subscribe(concreteObserver *subscriber)

Is subscriber every going to be NULL?
Do you need to check if subscriber has been destroyed?

Personally. I would pass by reference and document that a subject must live longer than the observers. Not that just because you pass by reference does not mean you can hold a pointer.

    void subscribe(concreteObserver& subscriber) // pass by reference
    {
        subscribers.push_back(&subscriber);      // but save pointer.
    }

But if you have a lot of dynamically allocated stuff this may not be suffecient. You could pass a std::weak_ptr then check of the subscriber is still available before sending the message.

###Use the new foreach loop

Use the new foreach loop

        std::vector<concreteObserver*>::const_iterator itr = subscribers.begin();
        while (itr != subscribers.end())
        {
            (*itr)->update(message);
            ++itr;
        }

Easier to write as:

        for(auto& sub: subscribers) {
           sub->update(message);
        }

###Inheritance is by default private

class concreteObserver : ObserverInterface 
                        ^^^^ Need public: here

Mark overriden methods:

void update(std::string message) // override ???

endl vs '\n'

Don't use endl unless you absolutely want to flush the buffer. Probably OK here. Just wanted to note it.

###Don't pass pointers:

    void subscribe(concreteObserver *subscriber)

Is subscriber every going to be NULL?
Do you need to check if subscriber has been destroyed?

Personally. I would pass by reference and document that a subject must live longer than the observers. Not that just because you pass by reference does not mean you can hold a pointer.

    void subscribe(concreteObserver& subscriber) // pass by reference
    {
        subscribers.push_back(&subscriber);      // but save pointer.
    }

But if you have a lot of dynamically allocated stuff this may not be suffecient. You could pass a std::weak_ptr then check of the subscriber is still available before sending the message.

###Use the new foreach loop

        std::vector<concreteObserver*>::const_iterator itr = subscribers.begin();
        while (itr != subscribers.end())
        {
            (*itr)->update(message);
            ++itr;
        }

Easier to write as:

        for(auto& sub: subscribers) {
           sub->update(message);
        }

Inheritance is by default private

class concreteObserver : ObserverInterface 
                        ^^^^ Need public: here

Mark overriden methods:

void update(std::string message) // override ???

endl vs '\n'

Don't use endl unless you absolutely want to flush the buffer. Probably OK here. Just wanted to note it.

Don't pass pointers:

    void subscribe(concreteObserver *subscriber)

Is subscriber every going to be NULL?
Do you need to check if subscriber has been destroyed?

Personally. I would pass by reference and document that a subject must live longer than the observers. Not that just because you pass by reference does not mean you can hold a pointer.

    void subscribe(concreteObserver& subscriber) // pass by reference
    {
        subscribers.push_back(&subscriber);      // but save pointer.
    }

But if you have a lot of dynamically allocated stuff this may not be suffecient. You could pass a std::weak_ptr then check of the subscriber is still available before sending the message.

Use the new foreach loop

        std::vector<concreteObserver*>::const_iterator itr = subscribers.begin();
        while (itr != subscribers.end())
        {
            (*itr)->update(message);
            ++itr;
        }

Easier to write as:

        for(auto& sub: subscribers) {
           sub->update(message);
        }
Source Link
Loki Astari
  • 96.6k
  • 5
  • 125
  • 338

###Inheritance is by default private

class concreteObserver : ObserverInterface 
                        ^^^^ Need public: here

Mark overriden methods:

void update(std::string message) // override ???

endl vs '\n'

Don't use endl unless you absolutely want to flush the buffer. Probably OK here. Just wanted to note it.

###Don't pass pointers:

    void subscribe(concreteObserver *subscriber)

Is subscriber every going to be NULL?
Do you need to check if subscriber has been destroyed?

Personally. I would pass by reference and document that a subject must live longer than the observers. Not that just because you pass by reference does not mean you can hold a pointer.

    void subscribe(concreteObserver& subscriber) // pass by reference
    {
        subscribers.push_back(&subscriber);      // but save pointer.
    }

But if you have a lot of dynamically allocated stuff this may not be suffecient. You could pass a std::weak_ptr then check of the subscriber is still available before sending the message.

###Use the new foreach loop

        std::vector<concreteObserver*>::const_iterator itr = subscribers.begin();
        while (itr != subscribers.end())
        {
            (*itr)->update(message);
            ++itr;
        }

Easier to write as:

        for(auto& sub: subscribers) {
           sub->update(message);
        }