Skip to main content
Code Review

Return to Answer

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
  • 97.7k
  • 5
  • 126
  • 341

###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);
 }
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /