Observer Design Pattern in C++ to model magazine subscriptions

Question

My attempt at a observer design pattern (I know using namespace std isn't good). I'm very new to design patterns. Everything is inline with class definition. Please tell me if I'm doing anything wrong or if there is anything that can be improved. Does subscriber (observer) need to store info from update function? Also, is operator== ok for comparing and finding correct subscriber with name or should I use getname()?

Base:

class Subscriber
{
    string name;
public:
    Subscriber(string name) : name(name) {}
    virtual void update(string title, double price) = 0;
    string getname() const {return name;}
    bool operator==(string name) {
        if (this->name == name)
            return true;
        return false;
    }
    virtual ~Subscriber() {};
};

Derived:

class PaidSubscriber : public Subscriber
{
public:
    PaidSubscriber(string name) : Subscriber(name) {}
    void update(string title, double price) {
        cout << "\nPaid Subscriber: " << getname() << endl;
        cout << "New Issue: " << title << endl;
        if (price > 0)
            cout << "Price: $" << std::fixed << setprecision(2) << price << endl;
        else 
            cout << "Price: FREE";
    };
};

class VIPSubscriber : public Subscriber
{
public:
    VIPSubscriber(string name) : Subscriber(name) {}
    void update(string title, double price) {
        cout << "\nVIP Subscriber: " << getname() << endl;
        cout << "New Issue: " << title << endl;
        if (price-10 > 0)
            cout << "Price: $" << std::fixed << setprecision(2) << price-10 << endl;
        else 
            cout << "Price: FREE";
    };
};

class FreeSubscriber : public Subscriber
{
public:
    FreeSubscriber(string name) : Subscriber(name) {}
    void update(string title, double price) {
        cout << "\nFree Subscriber: " << getname() << endl;
        cout << "New Issue: " << title;
        if (price > 0) {
            cout << " (preview only)" << endl;
            cout << "Upgrade to Paid Subscriber!" << endl;
        }
        else 
            cout << endl;
    };

};

Subject:

class Magazine
{
    string currissue;
    double currprice;
    
    list<Subscriber*>sublist;
public:
    enum SubscriberType {SUB_FREE, SUB_PAID, SUB_VIP};
    Magazine(string currissue, double currprice) : currissue(currissue), currprice(currprice) {}
    void subscribe(string name, SubscriberType subtype) {
        Subscriber *sub; 
        switch(subtype) {
            case SUB_FREE: 
                sub = new FreeSubscriber(name);
                break;
            case SUB_PAID:
                sub = new PaidSubscriber(name);
                break;
            case SUB_VIP:
                sub = new VIPSubscriber(name);
                break;
            default:
                return;
        }
        sublist.push_back(sub);
    }
    void unsubscribe(string name) 
    {
        for (auto it = sublist.begin(); it != sublist.end(); it++) {
            if (**it == name) {
                sublist.erase(it);
                break;
            }
        }
    }
    void changeissue(string name, double price) {
        this->currissue = name;
        this->currprice = price;
    }
    void notify()
    {
        for (auto sub : sublist)
            sub->update(currissue, currprice);
    }
    ~Magazine() {
        for (auto sub : sublist)
            delete sub;
    }
};

Main:

int main()
{
    Magazine nationalgeographic("Lions", 5.34);
    nationalgeographic.subscribe("Ivy Parks", Magazine::SUB_PAID);
    nationalgeographic.subscribe("Mike Gopher", Magazine::SUB_FREE);
    nationalgeographic.subscribe("Stan Shunpike", Magazine::SUB_VIP);
    nationalgeographic.notify();

    nationalgeographic.changeissue("Elephants", 15.23);
    nationalgeographic.notify();
}

Answer 1

Answers to your questions

I know using namespace std isn't good [...] Please tell me if I'm doing anything wrong or if there is anything that can be improved.

Well you already said it yourself: don't use using namespace std, especially not for anything you would put into a header file.

Does subscriber (observer) need to store info from update function?

That depends on what the observer needs to do. If you just want to print some information like in your example, there is no need to store anything.

Also, is operator== ok for comparing and finding correct subscriber with name or should I use getname()?

I recommend using getname(). Use operator== primarily to compare two objects of the same type to each other. So I would rewrite it to:

bool operator==(const Subscriber& other) const {
    return this->name == other.name;
}

However, there is not much use for this in your code, so you could also consider removing it.

Return booleans directly

Instead of writing:

if (foo)
    return true;
else
    return false;

You can write this instead:

return foo;

Use '\n' instead of std::endl

Prefer using '\n' instead of std::endl; the latter is equivalent to the former, but also forces the output to be flushed, which is usually not necessary and impacts performance.

Pass strings by const reference where appropriate

A std::string can be a large object that allocates memory on the heap. By passing them by value to other functions, a copy has to be made. If you only need to read from the string, that copy is unnecessary and just reduces performance. To avoid this, pass them by const reference instead. For example:

class Subscriber
{
    std::string name;
public:
    Subscriber(const std::string& name) : name(name) {}
    ...
};

If you can use C++17, then even better would be to use std::string_view to pass strings.

Avoid manual new and delete

Manually calling new and delete makes it easy for errors to slip into your code. For example, if you unsubscribe a Subscriber, you only erase its pointer in sublist, you forgot to actually delete the Subscriber object. So avoid manual new and delete whenever you can. In the case of the list of subscribers, you could make it a list of std::unique_ptrs:

std::list<std::unique_ptr<Subscriber>> sublist;

Your subscribe() function could be changed to:

void subscribe(const std::string& name, SubscriberType subtype) {
    std::unique_ptr<Subscriber> sub; 

    switch (subtype) {
    case SUB_FREE: 
        sub = std::make_unique<FreeSubscriber>(name);
        break;
    ...
    }

    sublist.push_back(std::move(sub));
}

Although there are nicer solutions which I will show below.

In notify(), you need to ensure you use a reference:

for (auto& sub: sublist)
    sub->update(currissue, currprice);

And ~Magazine() can now be removed completely; when sublist is destroyed, it will in turn destroy all std::unique_ptrs, which in turn will delete the objects they hold.

Simplifying subscribe()

The problem with subscribe() is that it not only has to add a subscriber to the list, it also needs to create a new subscriber object based on subtype. If you add lots of different subscriber types, that means this function will grow a lot as well. It would be nice to keep this function small and only have to deal with adding a subscriber to the list. Ideally, it looks like:

void subscribe(std::unique_ptr<Subscriber>&& sub) {
    sublist.push_back(std::move(sub));
}

Then in main() you would have to write:

nationalgeographic.subscribe(std::make_unique<PaidSubscriber>("Ivy Parks"));

In a way, that just moves the problem to the caller. A possible way to make the caller simpler as well is to still have subscribe() create the Subscriber objects, but then to make it a template to avoid code repetition:

template<class SubscriberType>
void subscribe(const std::string& name) {
    sublist.push_back(std::make_unique<SubscriberType>(name));
};

Then the caller looks like:

nationalgeographic.subscribe<PaidSubscriber>("Ivy Parks");

Merge changeissue() and notify()

Whenever a new issue is released by the magazine, you always want to notify the subscribers. So it doesn't make sense to have two separate functions that you need to call. I would create one function instead:

void release_issue(const std::string& issue, double price) {
    for (auto& sub: sublist)
        sub->update(issue, price);
}

With this, you also no longer need the member variables currissue and currprice, and the constructor can be removed. You would use it like so:

Magazine nationalgeographic;
nationalgeographic.subscribe<PaidSubscriber>("Ivy Parks");
...
nationalgeograpic.release_issue("Lions", 5.34);
nationalgeograpic.release_issue("Elephants", 15.23);

Answer 2

As already mentioned in my coment you should add and remoove subscribers by their pointer and not name. Also use a set instead om list. So here a changes I would do then:

set<Subscriber*> subscribers;

void subscribe(Subscriber* subscriber) //add Subscriber
{
  subscribers.insert(subscriber);
}

void unsubscribe(Subscriber* subscriber) //remove Subscriber
{
  subscribers.erase(subscriber);
}

I am also using just C style pointers here, but it could be a good idea to use some kind of smartpointer, not shure which one of that smartpointers feats the best here. If you removing subscribers by name, you could remove the wrong one. If you wrote this example to get familiar with design patterns, you could use an other pattern for creating concrete subscribers, Factory Method Pattern.

Answer 3

Please consider the possibility of more types of subscribers in future. To immune or have least impact on the existing code, you can use Factory Pattern to create subscriber as followed:

class SubscriberFactory {
 public:
   static Subscriber* createSubscriber(SubscriberType subtype, string name) {
     switch(subtype) {
        case SUB_FREE: 
            return new FreeSubscriber(name);
        case SUB_PAID:
            return new PaidSubscriber(name);
        case SUB_VIP:
            return new VIPSubscriber(name);             
        default:
            /* This way you also adhere to RAII. */
            throw std::invalid_argument("Invalid subscriber type");
            return nullptr; 
   }
};

With this, Magazine::subscribe() would be as followed:

    class Magazine {
       string currissue;
       double currprice;
       list<Subscriber*>sublist;
      public:
         void subscribe(string name, SubscriberType subtype) {
           Subscriber *sub = SubscriberFactory::createSubscriber(subtype, name);
           sublist.push_back(sub);
         }
    };