Observer pattern implementation in C++11

Question

Can this observer pattern implementation be improved?

Problem:

User will drive a car if pressed l --> car move left c--> move middle ,r--> move right

#include<iostream>
#include<vector>
#include<algorithm>
// -1 0 1
class Car // subject
{
 int position;
 std::vector<class observer*>observerList;
 public:
 int getPosition()
 {
 return position;
 }
 void setPosition(int newpos)
 {
 position =newpos;
 notify();
 }
 void attach(observer *obs)
 {
 observerList.push_back(obs);
 }
 void detach(observer *obs)
 {
 observerList.erase(std::remove(observerList.begin(),observerList.end(),obs),observerList.end());
 }
 void notify();
};
class observer
{
 Car *_car;
 public:
 observer(Car *car)
 {
 _car=car;
 _car->attach(this);
 }
 virtual void update()=0;
 protected :
 Car* getCar()
 {
 return _car;
 }
};
void Car::notify()
{
 if(observerList.empty())
 return;
 for(int i=0;i< observerList.size(); ++i)
 {
 observerList[i]-> update();
 }
}
class LeftObserver:public observer
{
 public: LeftObserver(Car *car):observer(car){}
 void update()
 {
 int pos = getCar()->getPosition();
 if(pos<0)
 {
 std::cout<<"Left side"<<"\n";
 }
 }
};
class RightObserver : public observer
{
 public: RightObserver(Car *car):observer(car) {}
 void update()
 {
 int pos=getCar()->getPosition();
 if(pos > 0)
 {
 std::cout<<"Right side"<<"\n";
 }
 }
};
class Middleobserver : public observer
{
 public: Middleobserver(Car * car):observer(car){}
 void update()
 { int pos=getCar()->getPosition();
 if(pos==0)
 {
 std::cout<<"Middle side"<<"\n";
 }
 }
};
int main()
{
 Car *car = new Car(); //subject
 LeftObserver letObserver(car); // one to many relationship
 RightObserver rigthObserver(car);
 Middleobserver middleObsever(car);
 std::cout<<"hit left,right,middle to drive a car,press Esc to close"<<"\n";
 char pressedButton;
 bool breakLoop= false;
 while(breakLoop==false)
 {
 std::cin>>pressedButton;
 switch(pressedButton)
 {
 case 108: // l ->pressed for left
 {
 car->setPosition(-1);
 break;
 }
 case 99: // c ->pressed for middle 
 {
 car->setPosition(0);
 break;
 }
 case 114: // r ->pressed for right
 {
 car->setPosition(1);
 break;
 }
 case 98: // b ->pressed for left
 {
 breakLoop= true;
 break;
 }
 default :
 {
 std::cout<<"please drive carefully"<<std::endl;
 break;
 }
 }
 }
 std::cout<<"Bye"<<"\n";
}

• DNKpp: Just a small recommendation: prefer references over pointers, if you don't want to make the parameter optional. You should have checked all your observer* if they are nullptr or not. Do not trust input!

Answer 1

Don't use push_back on vectors, basically ever

emplace_back() is almost always preferable.

Slow removal

Ideally, you'd want to store the observer list in an intrusive doubly-linked list, so that you can have O(1) removal. Unfortunately, this type of pattern is a bit of a blind spot in the STL, as there is no ideal container for it. (AFAIK, someone please correct me on this if I overlooked something).

if you don't have access to something like boost::intrusive::list, you can get around this by storing a std::list<Observer*>::iterator in the Observer itself. You "could" manually set up the list, but it's rarely worth the trouble.

Naming: Do not call a vector somethingList.

"List" is loaded with meaning, and implies guarantees that your code does not provide, so it's misleading

I would personally just use observers for the vector.

RAII

Since you attach the observer to the car in Observer's constructor, you should detach it in the destructor.

Consider using std::function instead of polymorphism for callbacks

Consider how much more flexible your code would be if observers was a: std::list<std::function<void(Car const&)>>.

Specifically, once you start scaling usage of this pattern up, you will find yourself with classes inheriting from 4-5 different listener interfaces, which will lead you to realize that update() is an awkward name, and a bunch of different headaches.

Here's a rough example to get you started: #include #include

class Car {
 std::vector<std::function<void(Car const&)>> observers_;
public:
 void attach(std::function<void(Car const&)> cb) {
 observers_.emplace_back(std::move(cb));
 }
 void update() {
 for(auto const& obs : observers_) {
 obs(*this);
 }
 }
};
class Whatever {
public:
 Whatever(Car* car) {
 car->attach([this](Car const& c) {onCarUpdate(c);});
 }
private:
 void onCarUpdate(Car const&) {
 }
};

• rim: Thank you but it seem there is compile time error in your suggested rough example at line car->attach([this](car const& c) {onCarUpdate(c);}) .Can you please review it ?
• user128454: Ysurf sorry about that, it should be fine now.