patterncppMinor
Logging object for C++
Viewed 0 times
loggingobjectfor
Problem
I've been always updating my log methods, because I started SDL and I needed some new methods. First, I made difference between debug and release, and so console and file. But now I want a more powerful method, which handles the console with implicating the file, and works like C++ streams. I would like to know if it's a good design.
So, is this a good, safe m
#ifndef LOGGING_INCLUDED
#define LOGGING_INCLUDED
#include
#include
#include
#include
namespace BSSE_System
{
/*bool addStream(std::ostream* os);
bool addStream(std::ostream* os, unsigned short& channel);
void doLog(const unsigned short& channel, const std::string& message, bool doTime = true);*/ //on specific channel;
/*void doLog(const std::string& message, bool doTime = true);*/ //on all channels
class LogObject
{
public:
LogObject()
{
streams.resize(0);
doTime = true;
}
bool addStream(std::ostream* os)
{
if (os == NULL)
{
return false;
}
else
{
streams.push_back(os);
return true;
}
}
bool addStream(std::ostream* os, unsigned short& channel)
{
if (os == NULL)
{
return false;
}
else
{
streams.push_back(os);
channel = streams.size() - 1;
return true;
}
}
void set_printTime(const bool& state)
{
doTime = state;
}
template
friend LogObject& operator::iterator it = lo.streams.begin();
it tm_hour tm_min tm_sec tm_hour
tm_min tm_sec streams;
bool doTime;
bool timeSwitch;
};
extern LogObject logger;
}
#endif // LOGGING_INCLUDEDextern LoggerClass logger; //defined in cpp like for std's cout and cinSo, is this a good, safe m
Solution
I see some things that may help you improve your code.
Use smart pointers
The design as currently implemented has a serious problem -- the raw pointers being stored might not actually point to live objects when output is attempted. To fix this, the code should instead
Be careful with template bloat
The way the current
Would cause three different versions of that code to be generated. If you are only logging a few types, that might not be bad, but I'd recommend instead having the
Use
If your compiler is C++11 compliant, you can use
You can write this:
Use "range-for" to simplify your code
Instead of using explicit iterators, you can use "range-for" and simplify your code. For example, the
Move loop invariants outside the loop
In the
Use smart pointers
The design as currently implemented has a serious problem -- the raw pointers being stored might not actually point to live objects when output is attempted. To fix this, the code should instead
std::shared_ptr rather than raw pointers.Be careful with template bloat
The way the current
operator<< object is written, a new function will be generated for every different class that is logged. In other words, this code:log << "mystery";
log << 42;
log << 98.6;Would cause three different versions of that code to be generated. If you are only logging a few types, that might not be bad, but I'd recommend instead having the
logger take a const std::string& as an argument and having the caller do whatever is required to turn the message into a string.Use
std::put_timeIf your compiler is C++11 compliant, you can use
std::put_time and make your code considerably simpler. Instead of this:**it tm_hour tm_min tm_sec << " - ";You can write this:
**it << std::put_time(timeStruct, "%T - ");Use "range-for" to simplify your code
Instead of using explicit iterators, you can use "range-for" and simplify your code. For example, the
operator<< code could be written like this:template
friend LogObject& operator<< (LogObject& lo, const T& message)
{
for (auto &out : lo.streams) {
if (lo.doTime) {
time_t rawTime = time(NULL);
tm * timeStruct = localtime(&rawTime);
*out << std::put_time(timeStruct, "%T - ");
}
*out << message;
}
return lo;
}Move loop invariants outside the loop
In the
operator << code above note that the time is still being recalculated every loop iteration. Unless IO is really slow or you have lots of output log streams or both, this probably isn't necessary. Instead, you could calculate time once and then use the precalculated time string. I'd write it like this:template
friend LogObject& operator<< (LogObject& lo, const T& message)
{
std::string timestring = "";
if (lo.doTime) {
time_t rawTime = time(NULL);
tm * timeStruct = localtime(&rawTime);
std::stringstream tstr;
tstr << std::put_time(timeStruct, "%T - ");
timestring = tstr.str();
}
for (auto &out : lo.streams) {
*out << timestring << message << '\n';
}
return lo;
}Code Snippets
log << "mystery";
log << 42;
log << 98.6;**it << std::setw(2) << timeStruct->tm_hour << ':'
<< std::setw(2) << timeStruct->tm_min << ':'
<< std::setw(2) << timeStruct->tm_sec << " - ";**it << std::put_time(timeStruct, "%T - ");template <typename T>
friend LogObject& operator<< (LogObject& lo, const T& message)
{
for (auto &out : lo.streams) {
if (lo.doTime) {
time_t rawTime = time(NULL);
tm * timeStruct = localtime(&rawTime);
*out << std::put_time(timeStruct, "%T - ");
}
*out << message;
}
return lo;
}template <typename T>
friend LogObject& operator<< (LogObject& lo, const T& message)
{
std::string timestring = "";
if (lo.doTime) {
time_t rawTime = time(NULL);
tm * timeStruct = localtime(&rawTime);
std::stringstream tstr;
tstr << std::put_time(timeStruct, "%T - ");
timestring = tstr.str();
}
for (auto &out : lo.streams) {
*out << timestring << message << '\n';
}
return lo;
}Context
StackExchange Code Review Q#121454, answer score: 7
Revisions (0)
No revisions yet.