Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird" "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

added more explanation of priority queue
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

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.

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.

Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

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 << " unable to load xml file " << static_cast<int>(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.

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.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /