5
\$\begingroup\$

I wrote a simple logger class for my project. Some key points:

  • The interface is heavily inspired by Java's logging libraries (you can tell from the getters/setters that I'm a Java programmer). In particular I'm using printf style functions instead of std::ostream style, which I don't like very much.
  • verify is a macro similar to assert, but instead of aborting it throws std::logic_error and cannot be disabled with NDEBUG.
  • The heavy lifting is done by Handler, which I didn't include here because it's quite a lot of code and it's not finished.

Without further ado, here's log.hpp

#pragma once

#include "verify.hpp"

#include <cassert>
#include <ostream>
#include <utility>

namespace bran {
    enum class log_severity : unsigned {
        INHERIT,
        TRACE,
        DEBUG,
        INFO,
        WARN,
        ERROR,
        FATAL
    };

    std::ostream& operator<<(std::ostream& out, log_severity severity);

    template<class Handler>
    class basic_logger;

    template<class Handler>
    void swap(basic_logger<Handler>& a, basic_logger<Handler>& b) noexcept;

    template<class Handler>
    class basic_logger {
    public:
        explicit basic_logger(const std::string& name, basic_logger<Handler>* parent, Handler* handler);
        explicit basic_logger(const std::string& name, basic_logger<Handler>& parent);
        explicit basic_logger(const std::string& name, Handler& handler);

        basic_logger(basic_logger&& l) noexcept;
        basic_logger(const basic_logger&) = delete;

        ~basic_logger() = default;

        basic_logger& operator=(basic_logger l) noexcept;

        [[nodiscard]] inline std::string get_name() const noexcept;

        void set_threshold(log_severity threshold);
        [[nodiscard]] inline log_severity get_threshold() const noexcept;

        void set_handler(Handler* handler);
        [[nodiscard]] inline Handler* get_handler() const noexcept;

        void set_parent(basic_logger<Handler>* parent);
        [[nodiscard]] inline basic_logger<Handler>* get_parent() const noexcept;

        [[nodiscard]] inline bool is_trace() const noexcept;
        [[nodiscard]] inline bool is_debug() const noexcept;
        [[nodiscard]] inline bool is_info() const noexcept;
        [[nodiscard]] inline bool is_warn() const noexcept;
        [[nodiscard]] inline bool is_error() const noexcept;

        template<class... T>
        inline void trace(const std::string& message, T... args);
        template<class... T>
        inline void debug(const std::string& message, T... args);
        template<class... T>
        inline void info(const std::string& message, T... args);
        template<class... T>
        inline void warn(const std::string& message, T... args);
        template<class... T>
        inline void error(const std::string& message, T... args);
        template<class... T>
        inline void fatal(const std::string& message, T... args);

    private:
        std::string name;
        log_severity threshold;
        basic_logger<Handler>* parent;
        Handler* handler;

        template<class... T>
        inline void log(log_severity severity, const std::string& message, T... args);

    public:
        friend void swap<Handler>(basic_logger<Handler>& a, basic_logger<Handler>& b) noexcept;
    };
}

template<class Handler>
bran::basic_logger<Handler>::basic_logger(const std::string& name, basic_logger<Handler>* parent, Handler* handler)
        : name{name}, parent{parent}, handler{handler} {
    verify(!name.empty() && (parent != nullptr || handler != nullptr));
    if (parent != nullptr) {
        threshold = log_severity::INHERIT;
    } else {
        threshold = log_severity::TRACE;
    }
}

template<class Handler>
bran::basic_logger<Handler>::basic_logger(const std::string& name, basic_logger<Handler>& parent)
        : basic_logger{name, &parent, nullptr} {
}

template<class Handler>
bran::basic_logger<Handler>::basic_logger(const std::string& name, Handler& handler)
        : basic_logger{name, nullptr, &handler} {
}

template<class Handler>
bran::basic_logger<Handler>::basic_logger(basic_logger&& l) noexcept {
    swap(*this, l);
}

template<class Handler>
bran::basic_logger<Handler>& bran::basic_logger<Handler>::operator=(basic_logger l) noexcept {
    swap(*this, l);
    return *this;
}

template<class Handler>
std::string bran::basic_logger<Handler>::get_name() const noexcept {
    return name;
}

template<class Handler>
void bran::basic_logger<Handler>::set_threshold(log_severity threshold) {
    verify(threshold != log_severity::INHERIT || parent != nullptr);
    this->threshold = threshold;
}

template<class Handler>
bran::log_severity bran::basic_logger<Handler>::get_threshold() const noexcept {
    assert(threshold != log_severity::INHERIT || parent != nullptr);
    return threshold == log_severity::INHERIT ? parent->get_threshold() : threshold;
}

template<class Handler>
void bran::basic_logger<Handler>::set_handler(Handler* handler) {
    verify(handler != nullptr || parent != nullptr);
    this->handler = handler;
}

template<class Handler>
Handler* bran::basic_logger<Handler>::get_handler() const noexcept {
    assert(handler != nullptr || parent != nullptr);
    return handler == nullptr ? parent->get_handler() : handler;
}

template<class Handler>
void bran::basic_logger<Handler>::set_parent(basic_logger<Handler>* parent) {
    verify(parent != nullptr || (handler != nullptr && threshold != log_severity::INHERIT));
    this->parent = parent;
}

template<class Handler>
bran::basic_logger<Handler>* bran::basic_logger<Handler>::get_parent() const noexcept {
    return parent;
}

template<class Handler>
bool bran::basic_logger<Handler>::is_trace() const noexcept {
    return get_threshold() <= log_severity::TRACE;
}

template<class Handler>
bool bran::basic_logger<Handler>::is_debug() const noexcept {
    return get_threshold() <= log_severity::DEBUG;
}

template<class Handler>
bool bran::basic_logger<Handler>::is_info() const noexcept {
    return get_threshold() <= log_severity::INFO;
}

template<class Handler>
bool bran::basic_logger<Handler>::is_warn() const noexcept {
    return get_threshold() <= log_severity::WARN;
}

template<class Handler>
bool bran::basic_logger<Handler>::is_error() const noexcept {
    return get_threshold() <= log_severity::ERROR;
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::trace(const std::string& message, T... args) {
    log(log_severity::TRACE, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::debug(const std::string& message, T... args) {
    log(log_severity::DEBUG, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::info(const std::string& message, T... args) {
    log(log_severity::INFO, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::warn(const std::string& message, T... args) {
    log(log_severity::WARN, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::error(const std::string& message, T... args) {
    log(log_severity::ERROR, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::fatal(const std::string& message, T... args) {
    log(log_severity::FATAL, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::log(log_severity severity, const std::string& message, T... args) {
    if (get_threshold() > severity) {
        return;
    }

    get_handler()->log(*this, severity, message, args...);
}

template<class Handler>
void bran::swap(basic_logger<Handler>& a, basic_logger<Handler>& b) noexcept {
    using std::swap;

    swap(a.name, b.name);
    swap(a.threshold, b.threshold);
    swap(a.parent, b.parent);
    swap(a.handler, b.handler);
}

And the puny log.cpp

#include "log.hpp"

std::ostream& bran::operator<<(std::ostream& out, bran::log_severity severity) {
    static const std::string SEVERITY_NAMES[] = {"INHERIT", "TRACE", "DEBUG", "INFO", "WARN", "ERROR", "FATAL"};
    static const size_t SEVERITY_COUNT = sizeof(SEVERITY_NAMES) / sizeof(SEVERITY_NAMES[0]);

    auto index = static_cast<size_t>(severity);
    assert(index < SEVERITY_COUNT);

    return out << SEVERITY_NAMES[index];
}
\$\endgroup\$
  • \$\begingroup\$ Did you know the existence of boost::log? \$\endgroup\$ – camp0 Oct 24 '19 at 7:51
  • \$\begingroup\$ @camp0 Yes. I don't like the interface. \$\endgroup\$ – Novotny Oct 24 '19 at 9:47
  • 1
    \$\begingroup\$ If you could show an example of how you intend to use this, it would be helpful to reviewers. As it is right now, you say everything is delegated to Handler, but the posted code never invokes Handler for any purpose. If you could supply those missing pieces, it would help a great deal. \$\endgroup\$ – Edward Oct 24 '19 at 12:58
  • \$\begingroup\$ Handler is used in the function basic_logger::log almost at the very end: get_handler()->log(*this, severity, message, args...); I'll post some example usage in a couple hours when I get home. \$\endgroup\$ – Novotny Oct 24 '19 at 13:27
3
\$\begingroup\$

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

Don't write getters and setters for every class

C++ isn't Java and writing getter and setter functions for every C++ class is not good style. Instead, move setter functionality into constructors and think very carefully about whether a getter is needed at all. In this code, there are some invariants being enforced, but I'd still be skeptical about the need for all of these setters and getters. For example, the whole chaining of raw pointers to loggers is rather suspect. While checks are made to verify the pointer isn't nullptr, a handler or parent could go out of scope and the class would still be using a non-nullptr but invalid pointer.

Minimize the interface

Is there really a compelling need to have an is_trace and related? I'd recommend simplifying the interface by omitting all of those. The user can just as easily write if(log.threshold <= log_severity::TRACE) {}.

Use better data structures

The code currently contains code for operator<< that looks like this:

std::ostream& bran::operator<<(std::ostream& out, bran::log_severity severity) {
        static const std::string SEVERITY_NAMES[] = {"INHERIT", "TRACE", "DEBUG", "INFO", "WARN", "ERROR", "FATAL"};
        static const size_t SEVERITY_COUNT = sizeof(SEVERITY_NAMES) / sizeof(SEVERITY_NAMES[0]);

    auto index = static_cast<size_t>(severity);
    assert(index < SEVERITY_COUNT);

    return out << SEVERITY_NAMES[index];
}

Since you're using the C++17 [[nodiscard]], you could also use std::string_view and std::array to make this cleaner and more modern.

#include <array>
#include <string_view>
std::ostream& bran::operator<<(std::ostream& out, bran::log_severity severity) {
    static constexpr std::array<std::string_view, 7> severity_names{"INHERIT", "TRACE", "DEBUG", "INFO", "WARN", "ERROR", "FATAL"};
    auto index = static_cast<size_t>(severity);
    assert(index < severity_names.size());
    return out << severity_names[index];
}

Also, it's better to avoid ALL_CAPS names. See ES.9

Rethink the existence of the class

As noted in the description of the code, all of the real work is being done by an unposted Handler function. Since the only useful things in the basic_logger class are the name and severity level, I'd suggest replacing the entire templated class with a concrete class that contains those two data items and whatever actual behavior is in Handler. The only required function in the Handler is apparently log. If you think you might need variations on the handler, simply make it a base class and make the log and destructor virtual.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people. Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code. This affects not only code reviews, but also maintenance of the code in the future, by you or by others. One good way to address that is by the use of comments. Another good technique is to include test code showing how your code is intended to be used.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your review. I don't understand what you mean by no invariant being enforced. Functions set_parent, set_severity and set_handler all use the verify macro (which I described in the OP) to prohibit certain object states. You cannot set parent to null without having a non-null handler, handler to null without having non-null parent and others. I want to be able to change the logging system at run-time from configuration file, that's why the variables are mutable. I didn't post handler, because I have several unfinished implementations and they are all rather big already and use... \$\endgroup\$ – Novotny Oct 24 '19 at 17:13
  • \$\begingroup\$ additional classes. The whole system is well over 1000 lines of code and again, unfinished. It supports logging configuration from files in several formats, synchronous and asynchronous handler, logging to console, file and TCP stream, triggers that perform some action when message matching certain pattern is logged and more. I decided instead of reviewing the whole mess, I could just get a review of the interface. The other points I have nothing to say against so again, thank you. \$\endgroup\$ – Novotny Oct 24 '19 at 17:18
  • \$\begingroup\$ @Novotny You're right -- I had overlooked the purpose of the verify as being a way to enforce runtime invariance. I'll update my answer. \$\endgroup\$ – Edward Oct 24 '19 at 17:28
1
\$\begingroup\$

I am not familiar with Java and I don't fully understand the purpose of Handler / parent - it would help us to better review the code if it was explained as pseudocode or via an interface.

  • Considering the way get_handler() is implemented... do you really intend to change parent's handler at runtime? Can't you just set the handler at class instantiation or something? Also, currently, changing parent at runtime is not thread-safe.

  • I doubt that you want your logger to be a template. If you use it across all your codebase then either all of the code is a template (which is very troublesome currently for developers and the compiler...) or you end up with using a single version of the logger. Instead consider usage of an interface class ILogHandler so that you could run applications with different handlers without them needing to know anything about it. Sure, calling a virtual function is slower than a regular function but it pales in comparison to everything else the logger needs to do upon a function call. Though, you'd need to implement in the logger what to do with T... args, not in the Handler.

  • Add verbose_level, not just severity_level. At times you'd want to see info_level log from a high level function but ignore warning/errors logs from low level functions.
  • Also consider writing implementation of short functions when declaring them. It doesn't hinder view of the whole class declaration and you can see the implementation without the need to search for it. I might be spoiled with Visual Studio but it allows to hide implementation of functions in its text editor so in this case even long functions provide little to hindrance when their implementation is written inside class declaration.
  • Since you work with C++17, consider using std::string_view instead of const std::string&.
  • Use std::forward<T>(args)... when you forward data in template functions as otherwise you might end up with unnecessary data copying:

    template<class Handler>
    template<class... T>
    void bran::basic_logger<Handler>::debug(const std::string& message, T... args) {
         log(log_severity::DEBUG, message, std::forward<T>(args)...);
    }
    
\$\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.