8
\$\begingroup\$

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";
}
\$\endgroup\$
  • \$\begingroup\$ 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! \$\endgroup\$ – DNKpp Dec 26 '17 at 23:27
6
\$\begingroup\$

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&) {
  }
};
\$\endgroup\$
  • \$\begingroup\$ 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 ? \$\endgroup\$ – rim Dec 27 '17 at 15:11
  • \$\begingroup\$ Ysurf sorry about that, it should be fine now. \$\endgroup\$ – Frank Dec 27 '17 at 15:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.