Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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.

#ifndef LOGGING_INCLUDED
#define LOGGING_INCLUDED

#include <ostream>
#include <vector>
#include <ctime>
#include <iomanip>

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 <typename T>
        friend LogObject& operator<< (LogObject& lo, const T& message)
        {
            for (std::vector<std::ostream*>::iterator it = lo.streams.begin();
                 it < lo.streams.end(); ++it)
            {
                if (lo.doTime)
                {
                    time_t rawTime = time(NULL);
                    tm * timeStruct = localtime(&rawTime);

                    **it << std::setw(2) << timeStruct->tm_hour << ':'
                        << std::setw(2) << timeStruct->tm_min << ':'
                        << std::setw(2) << timeStruct->tm_sec << " - ";
                }

                **it << message;
            }

            return lo;
        }

        void stringOut(const std::string& stringmes, const unsigned int channel)
        {
            if (doTime)
            {
                time_t rawTime = time(NULL);
                tm * timeStruct = localtime(&rawTime);

                *(streams.at(channel)) << std::setw(2) << timeStruct->tm_hour
                    << ':' << std::setw(2) << timeStruct->tm_min << ':'
                    << std::setw(2) << timeStruct->tm_sec << " - ";
            }

            *(streams.at(channel)) << stringmes;
        }

        void stringOut(const std::string& stringmes)
        {
            for (unsigned int i = 0; i < streams.size(); ++i)
            {
                stringOut(stringmes, i);
            }
        }

    private:
        std::vector<std::ostream*> streams;
        bool doTime;
        bool timeSwitch;
    };

    extern LogObject logger;
}

#endif // LOGGING_INCLUDED

extern LoggerClass logger; //defined in cpp like for std's cout and cin

So, is this a good, safe method for managing log? I can simply add pointers from iostream objects, and also from fstream objects after opening files. One problem is the time management. I currently don't have method, how to concatenate strings without problems on time (BUT this is not a main problem, it seems resolvable for me).

Also, it's different from classical operator<< overloads, because we need to define the "very left-hand" part of operation and not the data what is written in streams.

So, is it a good method, should I try to create an inherited ostream class or I should look for a different method?

share|improve this question
up vote 7 down vote accepted

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 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_time

If your compiler is C++11 compliant, you can use std::put_time and make your code considerably simpler. Instead of this:

**it << std::setw(2) << timeStruct->tm_hour << ':'
    << std::setw(2) << timeStruct->tm_min << ':'
    << std::setw(2) << timeStruct->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 <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;
}

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 <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;
}
share|improve this answer
    
You mean the ostream pointers should not be raw? That won't work, because you can't get a shared_ptr to e.g. std::cout. – immibis Feb 29 at 20:51
    
Yes, the stream objects are always destroyed and even closed, if I not declare via pointers. But it's a good answer. Thank you! – Lasoloz Mar 1 at 6:46
    
If you're going to use ostreams you don't own, see stackoverflow.com/questions/17220154/… – Edward Mar 1 at 7:17
    
@Edward put_time does not work, I need to update my gcc compiler version. Actually, I reimplemented in a different way as you said, but I use less variables. – Lasoloz Mar 3 at 17:51
    
If you're using gcc, you'll need to add -std=c++11 or -std=c++14 to the command line to get it to understand that you're using C++11 (or newer) features. Currently, if no dialect options are given, the default is -std=gnu++98 which is C++98 plus a few gnu extensions. – Edward Mar 3 at 18:53

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.