Implementing the observer pattern for a stock exchange scenario

Question

I am trying to implement the observer pattern for a stock exchange scenario. Multiple stocks can be registered at a stock exchange. There will be stock observers who are observing some specific stocks. Please advise how this implementation can be improved.

stock_exchange.hpp

#ifndef STOCK_EXCHANGE_HPP
#define STOCK_EXCHANGE_HPP

#include "stock_observer.hpp"
#include "stock.hpp"
#include<unordered_map>
#include<string>
#include<list>

class Stock_Observer;
class Stock;

class Stock_Exchange
{
    std::unordered_map<std::string, Stock*> registered_stocks;
    std::unordered_map<std::string, std::list<Stock_Observer*>> stock_observers;

    public:
        void notify_observers(std::string stock_name);
        void unregister_observer(Stock_Observer& so, std::string stock_name);
        void register_observer(Stock_Observer& so, std::string stock_name);
        void add_stock(Stock& new_stock);
        void remove_stock(Stock& s);
        Stock& get_stock(std::string stock_name);
};

#endif // STOCK_EXCHANGE_HPP

stock_exchange.cpp

#include "stock_exchange.hpp"
#include "stock_observer.hpp"
#include<algorithm>

void Stock_Exchange::add_stock(Stock& new_stock)
{
    registered_stocks[new_stock.get_name()] = &new_stock;
}

void Stock_Exchange::remove_stock(Stock& s)
{
    registered_stocks.erase(s.get_name());
    stock_observers.erase(s.get_name()); //Memory leak!!!
}

Stock& Stock_Exchange::get_stock(std::string stock_name)
{
    return *registered_stocks[stock_name];
}

void Stock_Exchange::register_observer(Stock_Observer& observer, std::string stock_name)
{
    stock_observers[stock_name].push_back(&observer);
}

void Stock_Exchange::unregister_observer(Stock_Observer& observer, std::string stock_name)
{
    auto l = stock_observers[stock_name];
    l.erase(std::find(l.begin(), l.end(),&observer));
}

void Stock_Exchange::notify_observers(std::string stock_name)
{
    for(auto o : stock_observers[stock_name])
    {
        o->notify(stock_name);
    }
}

stock_observer.hpp

#ifndef STOCK_OBSERVER_HPP
#define STOCK_OBSERVER_HPP

#include "stock.hpp"
#include "stock_exchange.hpp"
#include<string>

class Stock_Exchange;

class Stock_Observer
{
    static int id_sequence;
    int id;
    Stock_Exchange* se;
    public:
        Stock_Observer(Stock_Exchange* s)
        {
            se = s;
            id = id_sequence++;
        }
        void notify(std::string stock_name);
};

#endif // STOCK_OBSERVER_HPP

stock_observer.cpp

#include "stock_observer.hpp"
#include<iostream>

int Stock_Observer::id_sequence = 1;

void Stock_Observer::notify(std::string stock_name)
{
    std::cout << "Object " << id << " notified about " << stock_name << std::endl;
}

stock.hpp

#ifndef STOCK_HPP
#define STOCK_HPP
#include "stock_exchange.hpp"
#include<string>

class Stock_Exchange;

class Stock
{
    std::string name;
    double price;
    Stock_Exchange* se;

    public:
        Stock(Stock_Exchange* s, std::string stock_name, double stock_price)
        {
            se = s;
            name = stock_name;
            price = stock_price;
        }
        std::string get_name();
        void set_price(double p);

};
#endif // STOCK_HPP

stock.cpp

#include "stock.hpp"

std::string Stock::get_name()
{
    return name;
}

void Stock::set_price(double p)
{
    price = p;
    se->notify_observers(this->name);
}

main.cpp

#include<iostream>
#include "stock_exchange.hpp"
#include "stock_observer.hpp"
#include "stock.hpp"

int main()
{
    Stock_Exchange se;

    Stock_Observer so1(&se), so2(&se);

    Stock infy(&se,"INFY",1234.33);
    Stock tcs(&se,"TCS",1100);
    Stock sbi(&se,"SBI",2312.33);

    se.register_observer(so1,"INFY");
    se.register_observer(so2,"INFY");
    se.register_observer(so1,"TCS");

    infy.set_price(1250.33);
    tcs.set_price(999.32);
    sbi.set_price(2300.00);

}

Answer 1

Make immutable class members const

If you have a member of a class that is set once at construction time, but will never change, then make it const. For example, in Stock:

class Stock {
  const std::string name;
  ...
  public:
  Stock(..., std::string stock_name, ...): name(stock_name) {
    ...
  }

And once it is const, you can safely make name public and remove get_name().

Avoid circular references

Your stock exchanges contain stocks, but stocks also refer to stock exchanges. This circular reference is unnecessary and might give you problems; for example you had to add forward references to class Stock_Observer and class Stock in stock_exchange.hpp. Conceptually, a stock is not tied to a specific stock exchange, so just remove the member variable Stock::se.

Of course, the problem is now how to notify observers when the stock price changes. One way to handle that is to allow one to set a callback function in a Stock, for example:

#include <functional>

class Stock {
  std::function<void(Stock &)> price_changed_callback;
  double price;
  ...
  public:
  void set_price_changed_callback(std::function<void(Stock &)> callback) {
    price_changed_callback = callback;
  }

  void set_price(double p) {
    price = p;
    if (price_changed_callback)
      price_changed_callback(*this);
  }
  ...
}

Then the stock exchange calls set_price_changed_callback() when it registers the stock, and sets it to something like its notify_observers() function. If you want to have a Stock be part of multiple exchanges, then you'd better look for a library that handles more complex callback registration for you, like libsigc++.

Another option would be to move set_price() to the stock exchange itself.

Think carefully about ownership

In your code, stock exchanges have pointers to stocks, and stocks have a pointer to a stock exchange. But what if a stock goes out of scope before a stock exchange, or vice versa? It's not a problem in your simple main(), but once your code grows the chance you will make a mistake will increase. It's best to use the right tools to control ownership.

One possibility is to use shared_ptr<>. Make it so you can only create a shared_ptr to a Stock:

class Stock {
  private:
  Stock() { ... }

  public:
  static std::shared_ptr<Stock> make_stock(args) {
    return std::make_shared<Stock>(args);
  }
}

And have Stock_Exchange keep a map of shared_ptrs:

class Stock_Exchange {
  std::unordered_map<std::string, std::shared_ptr<Stock>> registered_stocks;
  ...
}

Alternatively, if you'd only ever have stocks associated with a single exchange, you could have Stock_Exchange take direct ownership:

class Stock_Exchange {
  std::unordered_map<std::string, Stock> registered_stocks;
  ...
  public:
  register_stock(std::string name, double price) {
    register_stocks.emplace(std::make_pair(name, Stock(name, price));
  }
}

Of course, if you go that route there is little point in having a class Stock anymore.

Use references when iterating over a container where possible

In this part of your code:

for(auto o : stock_observers[stock_name])
{
    o->notify(stock_name);
}

You are actually making a copy of each observer. It is almost always better to use a reference when iterating through a container:

for(auto &o : stock_observers[stock_name])
{
    o->notify(stock_name);
}