2
\$\begingroup\$

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);

}
\$\endgroup\$
4
\$\begingroup\$

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);
}
\$\endgroup\$

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.