WindMIll Data Publisher using observer pattern

Problem statement : There is a windmill which collects and stores wind data for various cities. The data is maintained in an xml file by the aggregator part of windmill. Now I am trying to implement the publisher part of the mill.
The responsibility of publisher is to notify the registered clients for their respective subscribed cities at registered interval of time for ex :

a. Client 1 registers for city DELHI and NOIDA at a refresh rate of 5 sec.

b. client 2 registers for city BANGLORE and MUMBAI at refresh rate of 3 sec.

The minimum refresh rate can be 1 Sec.
The publisher also reads/receives the data from the xml file at rate of 100 ms.
The publisher should notify clients about wind speed and wind direction.

Lets say the format of xml in which the data is stored is :

Where name is the name of city ,WindSpeed is the speed of wind and windDirection can 0,1,2,3 denoting NE,NW,SE,SW direction of wind.

My Solution : This seems to me to be an observer pattern type problem, Hence I took the help and overview of observer pattern from stackexchange answer.

The observer class :

EventType Denotes the city for which the WindData [ Event ] is being sent.

#ifndef _OBSERVER_
#define _OBSERVER_

struct Windata;
enum class EventType
{
    DELHI,
    NOIDA,
    BANGLORE,
    MUMBAI
};
class Observer
{
public:
    virtual void onEvent(EventType const &,Windata const &) = 0;
};
#endif

Also for the periodic timer the implementation is taken from this link of stackoverflow.

```
#include
#include
#include
#include

class CallBackTimer
{
public:
CallBackTimer()
:_execute(false)
{}

~CallBackTimer() {
if (_execute.load(std::memory_order_acquire)) {
stop();
};
}

void stop()
{
_execute.store(false, std::memory_order_release);
if (_thd.joinable())
_thd.join();
}
template
void start(int interval, callable&& f, arguments&&... args)
{
if (_ex

Here are some things that may help you improve your program.

Compile with warnings turned on

When I attempted to compile the project, it told me that this line was suspect:

if ((*clientitr)->handle = &observer)
{
    clientitr = clients.erase(clientitr);
}

Indeed, looking at the context, the condition within the if should be == instead of =. Your compiler can help you spot subtle bugs, so you should get into the habit of turning the warning levels all the way up.

Ensure every control path returns a proper value

The WindMill_Publisher::getCityData() routine returns a reference to a Windata structure under some set of conditions but then doesn't return anything at all otherwise. This is an error. The function must always return a valid reference. If it is not always possible, then the code should be changed to handle the error, as with returning a pointer and then having the caller check for nullptr.

Separate interface from implementation

The interface is the part in the .h file and the implementation is in the .cpp file. Users of this code should be able to read and understand everything they need from the implementation file. That means that, for example, the Subject.h file should be separated into two pieces: the interface (only) and the implementation (only). Having all of the code in the .h file defeats much of the purpose of having header files at all.

Fix spelling errors

The code has ClientDetials instead of ClientDetails and Banglore instead of Bangalore. These kinds of typos don't bother the compiler at all, but will bother human readers of the code and make it a little more difficult to understand and maintain.

Prefer throw rather than printing to cout

The current code includes this line within WindMill_Publisher::readCityData():

if(!res)
{
    std::cout (res.status)<<"\n";
    return false;
}

There are a few problems with this. First, the return value is never checked within the constructor, so the result is that the data may or may not have been correctly initialized. The second problem is that it's probably better and easier to simply throw an error from within the function. This helps assure that the created WindMill_Publisher object is valid if no exceptions were thrown.

Don't repeat yourself

The only difference between Client_1 and Client_2 is the data contained, so they should be different instances but the same class. This can easily be done by passing the required data in via a constructor. Even if your compiler doesn't support initializer lists, you can create an external function to allow adding of cities to the object.

Be careful with concurrency

As written, the code passes a reference to an Observer class to the registerClient class and then a pointer to that reference is stored within WindMill_Publisher. That's a serious problem because there is nothing to prevent the passed object from getting deleted after registration. The result is that the callback will attempt to reference a deleted object's member pointer which is not likely to be what you want. Instead, either create a copy and store that or use a std::shared_ptr.

Don't overcomplicate the design

There really isn't much need for the Observer class. All that's really required for the callback is a function pointer. This could be done just as well by passing in a freestanding function. It seems to me that this would make for a somewhat more flexible interface.

Consider a different approach

I find the code for the WindMill_Publisher to be much more complex than it needs to be. Fundamentally, there are a few different things happening. There is first, a data structure that holds current city data and this data structure is asynchronously updated from an external XML file. Next, there is the publisher of this code which sends updated data to each registered client. I would probably want to separate these a little more cleanly. Further, rather than waking up periodically to check the timers, this is a good candidate for a priority queue.

Here's how that might work. Calculate the deadlines for all notifications and store them in a priority queue. Then set the alarm for the item at the front of the queue. Example: if there are three items with deadlines that are 2, 5 and 7 seconds away, have the application sleep for 2 seconds. When it wakes, it handles the item at the top of the queue and recalculates. The next item is due 3 seconds later, so it sleeps for 3 seconds, etc. The queue then only needs to be adjusted when either a node is registered or unregistered and when the application wakes up. To avoid sleeping and then waking immediately, process the queue until there are no more items for the current second.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

N

StackExchange Code Review Q#139942, answer score: 3