Design Review
Ownership
One of the advances of C++ over is C is ownership semantics. We understand who owns an object (and thus who is responsible for destroying the object). We do this by using a set of helper classes that explicitly state ownership or pass by reference to indicate non ownership.
The trouble with pointers is that they have no ownership semantics. You can't tell from their usage who is supposed to own the pointer and thus you can not tell who is responsible for deleting the object. Thus in C++ it is highly discouraged to use RAW pointers in public interfaces (I am not saying are not used, I am saying is that their normally place is deep inside a class so their ownership is very obvious and not normally exposed in public interfaces).
In you interface you never transfer ownership and you have both a register and unregister method on the subject. So that interface can be easily changed to show the ownership is not part of the interface.
template <typename T>
class Subject {
public:
virtual void registerObserver(Observer<T>& ptr_observer) = 0;
virtual void unregisterObserver(Observer<T>& ptr_observer) = 0;
virtual void notifyObservers() = 0;
};
Please note I changed the *
to &
. This shows you are not passing ownership but you will always pass an observer. The additional upside is that you can no longer pass nullptr
as an observer (you forgot to check to see if the pointer was nullptr
and thus had a potential for calling a method on a nullptr
(not a good idea).
Passing by value:
Here you pass the subjectState
by value.
class Observer {
public:
virtual void update(T subjectState)=0;
};
If T is large or complex then this is probably not a good idea. Pass a reference to the object. If the subject wants a copy they can explicitly make a decision to copy the object when they receive it (they have enough context to make an intelligent choice). So pass by reference instead.
class Observer {
public:
virtual void update(T const& subjectState)=0;
// ^^^^^^ Normally I would pass by const ref.
// But there is a case for passing by ref
// as this allows each subject to mutate the
// state of the object that is detectable
// by other observers.
};
Virtual Destructors
If a class has virtual methods then you should have a virtual destructor.
The compiler will warn you about this if you turn up the warning level.
Override
When you override virtual methods you should add override
to the method declaration.
The compiler will warn you about this if you turn up the warning level.
Code Review
Prefer to use the range based for when looping over a container.
for (std::set<Observer<T>*>::iterator iter = observers.begin(); iter != observers.end(); iter++) {
(*iter)->update(state);
}
One thing you should change is the increment. It is slightly more efficient to use prefix increment (though the compiler probably compensates automatically). But it also makes your code look more modern.
for (std::set<Observer<T>*>::iterator iter = observers.begin(); iter != observers.end(); ++iter) {
(*iter)->update(state);
}
This works. But it is a bit verbose.
In C++11 we made this easier with auto
which makes the compiler do the work of working out the type.
for (auto iter = observers.begin(); iter != observers.end(); ++iter) {
(*iter)->update(state);
}
In C++14 we added std::begin()
and std::end()
functions to make the above more generic and work with arrays as well as standard containers.
for (auto iter = std::begin(observers); iter != std::end(observers); ++iter) {
(*iter)->update(state);
}
Also in C++14 we added the range based for:
The below loop is basically syntactic sugar (though it only calls std::end()
once in C++17) for the previous loop I wrote.
for (auto const& item: observers) {
item->update(state);
}
Const correctness.
Const correctness is really hard to add in afterwords (believe me). So best to start with your classes with it already in place. So any method that does not mutate the state of the object should be marked const
void stateChanged() const { // I added the cost here.
notifyObservers();
}
This class should be const and return by reference.
T const& getState() const { // Notice the 2 `const` on this.
return state;
}
Move semantics and efficiency.
Copy semantics are good for simple types. But move semantics allow you a very effecient way of adding an object to another object. So you should learn how to move or construct in place an object.
// Your version forces two copies.
// You have a copy when you call setState to put the object
// into the parameter `state` then you have a second copy
// when you assign to `this->state`
void setState(T state) { // Copy here
this->state = state; // Copy here
stateChanged();
}
If T was large thats a lot of extra copying.
You should replace this with three methods:
void setState(T const& newState) { // Pass by reference avoid copy.
state = state;
stateChanged();
}
void setState(T&& newState) { // Pass by RValue-Ref avoid copy.
state = std::move(newState); // Then move over current state.
stateChanged();
}
// Advanced technique
template<typename... A>
void setState(A&&... args) { // Pass arguments to construct a T by ref
state = T(std::forward<A>(args)...); // Build the state in place.
stateChanged();
}
Don't use this->
it hides errors.
this->state = state;
The only reason to use this->
is to distinguish a member that has been shadowed by a local variable (like a parameter). The problem is that it is error prone.
You must always remember to use this->
to use the local member otherwise you will accidentally use the local variable. If you forget there is no compiler warning that will help you.
On the other hand the compiler will warn you when you have shadowed variables. So if you never have a shadowed variable you can never accidentally use the wrong one.
Thus it is discouraged to use this->
as you are just setting yourself up for a hidden error. Rather make the compiler warn you about shadowed variables and use an appropriate name so it does not shadow.
Prefer '\n'
over std::endl
std::cout << "Testing Subject-Observer implementation:" << std::endl;
The difference between the two is a flush of the stream (done by std::endl
). Flushing done by the user is one of the main speed slow downs in C++ code over C. The streams will already flush themselves at the optimal time, so a manual flush will usually retard performance.
Prefer to use automatic variables.
ConcreteSubject<int>* ptr_subject = new ConcreteSubject<int>(42);
This is not Java. You don't need to new objects to create them you can just declare them locally.
// This creates a local object
ConcreteSubject<int> ptr_subject(42);
Using pointers like this (RAW pointers) is also discouraged as it very dangerous. If an exception occurs you will leak the pointer. You also have to keep track of the object and remember to call delete on it, which is error prone (as shown by a decades worth of C code). So we usually use wrappers to make sure the delete is called automatically.
std::unique_ptr<ConcreteSubject<int>> aPointer std::make_unique<ConcreteSubject<int>>(42);
This is a pointer held inside an object wrapper. When it goes out of scope it will automatically be destroyed, this calls the destructor, which calls delete on the contained pointer.
As an example:
ConcreteObserver<int>* ptr_observer_1 = new ConcreteObserver<int>(ptr_subject);
I see no call to delete for this object. So you just leaked it.