5
\$\begingroup\$

I'm making a personal logger and so far I have come up with 3 solutions. I have them simplified in this example to highlight my dilemma. I want to know the pros and cons of my approaches and ways to make them better. Also should I be flushing the stream as often as possible and why?

TempLog

This version will be created and destroyed on every log. Endline forced on destruction.

class TempLog
{
public:
    std::ostream& log()
    {
        return std::cout;
    }
    ~TempLog()
    {
        std::cout << "\n";
    }
};

SingletonLog_ver_1

If the endline is forced in this manner using the same ostream from outside the logger will result in bad output. Also it feels somewhat dishonest but due to my lack of experience I don't trust my feeling.

class SingletonLog_ver_1
{
public:
    static SingletonLog_ver_1 instance()
    {
        static SingletonLog_ver_1 instance;
        return instance;
    }

    std::ostream& log()
    {
        static bool firstLine = true;
        if (firstLine)
            firstLine = false;
        else
            std::cout << "\n";

        return std::cout;
    }

private:
    SingletonLog_ver_1() {};
};

SingletonLog_ver_2

Now while I like what is happening here more than the previous two examples it seems like the least elegant code. Is there another more elegant way to achieve this kind of functionality?

class SingletonLog_ver_2
{
public:
    static SingletonLog_ver_2 instance()
    {
        static SingletonLog_ver_2 instance;
        return instance;
    }

    std::pair<std::ostream&, std::unique_ptr<TempObj_NewLine>> log()
    {   
        return std::pair<std::ostream&, std::unique_ptr<TempObj_NewLine>>(std::cout, std::make_unique<TempObj_NewLine>());
    }

private:
    SingletonLog_ver_2() {};
};

TempObj_NewLine

class TempObj_NewLine
{
public:
    TempObj_NewLine() {};
    ~TempObj_NewLine()
    {
        std::cout << "\n";
    }
};

Use of the 3 loggers:

int main()
{
    TempLog().log() << "first log!" << " first log, same line.";
    TempLog().log() << "second log." << "second line.";

    SingletonLog_ver_2::instance().log().first << "first line singleton log!"<< " first line, again.";
    SingletonLog_ver_2::instance().log().first << "second line singleton log!";

    SingletonLog_ver_1::instance().log() << "first line singleton - hacky endline." << " first line, again.";
    SingletonLog_ver_1::instance().log() << "second line singleton - hacky endline";

    std::cin.get();
}
\$\endgroup\$
3
\$\begingroup\$

Singletons

There are several ways to make something a singleton. You are doing it by removing the public constructor, and forcing users to get an instance through a static member function. That works, but there are alternatives. You can create a regular class, and provide a global instance of that class. Just like so:

class Log {
    ...
};

Log logInstance;

You then just refer to logInstance instead of SingletonLog::instance().

Proper naming

Your class has a function log(), however that function does not log anything. Instead, it just returns a std::ostream reference. A more appropriate name for the function would then be get_stream().

If you intend to always use it as SingletonLog::instance().log() << "some string", then it might be better to just add an operator <<() overload to your logging class. This has several advantages:

  1. It's less typing: SingletonLog::instance() << "some string", or when using the alternative way of making a singleton: logInstance << "some string".
  2. You can actually read the contents of the string in your class, and check whether it ends with a newline or not.

Flushing

Whether or not to flush depends on what you want to log. If you log a lot of information, flushing adds considerable overhead. However, if you want to be sure that any errors that are logged are not lost when your program terminates abnormally, then it might be prudent to flush the output stream after every write to the log.

Other suggestions

Do you really want to use the << operator? If you rather like to use the printf() style, then consider making use of fmtlib, which provides a type-safe way to format strings the printf() or Python .format() way. Your logging class could look like:

#include <fmt/format.h>

class Log {
    template <typename... Args>
    void log(const std::string &format, const Args &... args) {
        std::cout << fmt::format(format, args...);
    }
};

Log logInstance;

And use it like:

logInstance.log("First line: {}\nSecond line: {}\n", "first entry", "second entry");
\$\endgroup\$
2
\$\begingroup\$

You chose a good implementation but then made something weird

Your Singleton implementation is a good one -it is the canonical implementation in C++ anyway, its main limitation being that it isn't thread safe. But you do something that appears nonsensical, which is returning a copy of the static instance initialized inside the instance() function, whereas the traditional technique is to return a reference. As far as I understand your code, you have as many instances of your logger as you have calls to instance(). Is that intended?

The canonical form is:

static S& getInstance() { // note the return by reference
    static S instance; 
    return instance;
}

Nice tricks but a lack of functionality

Then there are neat tricks to have new lines automatically added and such, but you don't offer more basic functionalities, such as choosing your ostream, and you can't decide on the time of flushing.

I believe that you should expand your design a bit. The singleton doesn't mean that you have to restrict yourself to a single logger. For instance, you could have several instances, returned by the like of error_log() (which would write to std::cerr and automatically flush), or event_log().

Do you really need to use a singleton?

Loggers are indeed, as far as I know, one of the more or less accepted use case of this very controversial design pattern. But are you certain that you need it? I use the likes of std::cout, std::cerr or std::clog, which are simple global objects, without encountering strange bugs. They won't be good enough if I need more functionality, like thread safety, time stamping and such, but neither will your proposed implementations. I am under the impression that you wanted to implement a Singleton more than a logging framework, whereas it should have been the opposite.

\$\endgroup\$
  • \$\begingroup\$ Not returning a reference is a lapsus. I've simplified the example as much as possible to get information on the problem i had at hand - getting good formating while having the logger a singleton. I am not sure do i really need to use a singleton - I guess one could always argue that I don't. I am a novice and it is true that I'm just trying as much different code as I come up with. As for the lack of functionality - I will put the full Logger up once i finish it. Thank you for your input :) \$\endgroup\$ – Antonio Dropulić Mar 19 '18 at 18:41
0
\$\begingroup\$

While the third solution SingletonLog_ver2 offers the desired functionality, It is hard to use/reuse and allows for misuse - assigning SingletonLog_ver_2::instance().log().second and thus avoiding destruction.

Solution bellow works around those things by delegating its ostream. I have changed class names for better readablility.

class StreamDelegate
{
public:
    StreamDelegate(std::ostream& os)
        :
        os(os)
    {}

    ~StreamDelegate()
    {
        os << "\n";
    }
    template<class T>
    StreamDelegate& operator<<(T&& output)
    {
        os << std::forward<T>(output);
        return *this;
    }

    StreamDelegate(const StreamDelegate&) = delete;
    StreamDelegate& operator=(const StreamDelegate&) = delete;

    StreamDelegate(StreamDelegate&&) = default;
    StreamDelegate& operator=(StreamDelegate&&) = default;

private:
    std::ostream& os;
};

class SingletonLog
{
public:
    static SingletonLog& instance(std::ostream& os)
    {
        static SingletonLog instance(os);
        return instance;
    }


    class StreamDelegate log()
    {

        return StreamDelegate(os);
    }



private:
    SingletonLog(std::ostream& os)
        :
        os(os)
    {
    }

    std::ostream& os;
};


int main()
{

    SingletonLog::instance(std::cout).log() << "First line: first entry. " << "First line: second entry";
    SingletonLog::instance(std::cout).log() << "Second line";

    std::cin.get();
}

This solution has been provided on Stack Overflow.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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