C++ logging class

Question

I've created a simple C++ logging class. It outputs various loglevels, and adjusts output color accordingly. I'm interested in any tips to improve my coding style.

I am especially concerned with my use of #define's in the bottom of Logger.hpp. I was unable to find another way to pass __FILE__ and __LINE__ to the function (aside from doing it manually ofcourse), but I would not be suprised if this approach is bad practise.

ConsoleColor.hpp

#include <Windows.h>

namespace AGE
{
    enum class BackgroundColor : int
    {
        Black   = 0,
        Blue    = 16,
        Green   = 32,
        Cyan    = 48,
        Red     = 64,
        Magenta = 80,
        Brown   = 96,
        White   = 112,
    };

    enum class ForegroundColor : int
    {
        Black = 0,
        Blue,
        Green,
        Cyan,
        Red,
        Magenta,
        Brown,
        White,
        Gray,
        Intense_blue,
        Intense_green,
        Intense_Cyan,
        Intense_red,
        Intense_magenta,
        Yellow,
        Intense_white,
    };

    // Used to combine background and foreground colors
    WORD operator &(const BackgroundColor& left, const ForegroundColor& right)
    {
        return static_cast<WORD>(static_cast<int>(left) + static_cast<int>(right));
    }
}

LoggerSettings.hpp

#pragma once

namespace AGE
{
    struct LoggerSettings
    {
        explicit LoggerSettings(bool should_print_level = true, bool should_print_file_and_line = true,
                                bool should_print_color = true, bool should_print_time = true) :
        print_level(should_print_color),
        print_file_and_line(should_print_file_and_line),
        print_color(should_print_color),
        print_time(should_print_time)
        {}

        // TODO add ability to display time in Logger.hpp/cpp
        bool print_level;
        bool print_file_and_line;
        bool print_color;
        bool print_time;
    };
}

Logger.hpp

#pragma once

#include "LoggerSettings.hpp"

#include <Windows.h>
#include <string>

namespace AGE
{
    enum class LogLevel
    {
        Trace,
        Info,
        Warn,
        Error,
        Fatal
    };



    class Logger
    {
    public:
        Logger();

        void set_log_level(const LogLevel level);

        LoggerSettings& get_settings();

        void explicit_trace(const std::string_view message, const char* file, int line);
        void explicit_info (const std::string_view message, const char* file, int line);
        void explicit_warn (const std::string_view message, const char* file, int line);
        void explicit_error(const std::string_view message, const char* file, int line);
        void explicit_fatal(const std::string_view message, const char* file, int line);

    private:
        void log(const LogLevel level, const std::string_view message, const char* file, int line);

        WORD get_color(const LogLevel level);
        std::string get_error_string(const LogLevel level);
        std::string get_file_name(const char* filepath);

        const HANDLE m_console_handle;
        LogLevel     m_current_log_level;
        LoggerSettings m_settings;
    };
}

#define trace(message)  AGE::Logger::explicit_trace(message, __FILE__, __LINE__)
#define info(message)   AGE::Logger::explicit_info(message, __FILE__, __LINE__)
#define warn(message)   AGE::Logger::explicit_warn(message, __FILE__, __LINE__)
#define error(message)  AGE::Logger::explicit_error(message, __FILE__, __LINE__)
#define fatal(message)  AGE::Logger::explicit_fatal(message, __FILE__, __LINE__)

Logger.cpp

#include "Logger.hpp"

#include "ConsoleColor.hpp"

#include <iostream>

#include <cstdint>

namespace AGE
{
    Logger::Logger() :
    m_console_handle   (GetStdHandle(STD_OUTPUT_HANDLE)),
    m_current_log_level(LogLevel::Trace)
    {
        m_settings = LoggerSettings();
    }



    void Logger::set_log_level(LogLevel level)
    {
        m_current_log_level = level;
    }



    LoggerSettings& Logger::get_settings()
    {
        return m_settings;
    }



    void Logger::explicit_trace(const std::string_view message, const char* file, int line)
    {
        if (m_current_log_level > LogLevel::Trace)
        {
            return;
        }

        log(LogLevel::Trace, message, file, line);
    }



    void Logger::explicit_info(const std::string_view message, const char* file, int line)
    {
        if (m_current_log_level > LogLevel::Info)
        {
            return;
        }

        log(LogLevel::Info, message, file, line);
    }



    void Logger::explicit_warn(const std::string_view message, const char* file, int line)
    {
        if (m_current_log_level > LogLevel::Warn)
        {
            return;
        }

        log(LogLevel::Warn, message, file, line);
    }



    void Logger::explicit_error(const std::string_view message, const char* file, int line)
    {
        if (m_current_log_level > LogLevel::Error)
        {
            return;
        }

        log(LogLevel::Error, message, file, line);
    }



    void Logger::explicit_fatal(const std::string_view message, const char* file, int line)
    {
        if (m_current_log_level > LogLevel::Fatal)
        {
            return;
        }

        log(LogLevel::Fatal, message, file, line);
    }



    void Logger::log(const LogLevel level, const std::string_view message, const char* file, int line)
    {

        if (m_settings.print_color)
        {
            WORD color = get_color(level);
            SetConsoleTextAttribute(m_console_handle, color);
        }
        if (m_settings.print_level)
        {
            std::cout << "[" << get_error_string(level) << "]";
        }
        if (m_settings.print_file_and_line)
        {
            std::cout << "(" << "In " << get_file_name(file) << " at line " << line << ") ";
        }

        std::cout << message << std::endl;
        SetConsoleTextAttribute(m_console_handle, BackgroundColor::Black & ForegroundColor::White);
    }



    WORD Logger::get_color(const LogLevel level)
    {
        switch (level)
        {
        default:
            return BackgroundColor::Black & ForegroundColor::White;
            break;

        case LogLevel::Trace:
            return BackgroundColor::Black & ForegroundColor::Intense_white;
            break;

        case LogLevel::Info:
            return BackgroundColor::Black & ForegroundColor::Intense_green;
            break;

        case LogLevel::Warn:
            return BackgroundColor::Black & ForegroundColor::Intense_red;
            break;

        case LogLevel::Error:
            return BackgroundColor::Black & ForegroundColor::Intense_Cyan;
            break;

        case LogLevel::Fatal:
            return BackgroundColor::Black & ForegroundColor::Intense_magenta;
            break;
        }
    }



    std::string Logger::get_error_string(const LogLevel level)
    {
        switch (level)
        {
        default:
            return std::string();
            break;

        case LogLevel::Trace:
            return std::string("Trace");
            break;

        case LogLevel::Info:
            return std::string("Info");
            break;

        case LogLevel::Warn:
            return std::string("Warning");
            break;

        case LogLevel::Error:
            return std::string("Error");
            break;

        case LogLevel::Fatal:
            return std::string("Fatal");
            break;
        }
    }



    std::string Logger::get_file_name(const char* filepath)
    {
        std::string path = std::string(filepath);

        int index = 0;
        for (int i = static_cast<int>(path.size()) - 1; i > 0; i--)
        {
            if (path.at(i) == '\\')
            {
                index = i;
                break;
            }
        }

        // For some reason the compiler complains if i dont cast to 64bit int
        return std::string(path, static_cast<int64_t>(index) + 1, path.size() - 1);
    }
}

Answer 1

You lack several features:

• logging to files, not just CMD; to this end you'd better make an interface class for logger and pass only pointer to the interface. Otherwise you'll end up with too many if/else and hard-to-maintain code.
• option for printing the time of the message. Even the C++ style support isn't available in C++17 there is a C-style version you can use.
• your logger is not suitable for multi-threading environment (std::cout thread safe but won't necessarily print characters in the order you desire).
• consider switching << std::endl; with << "\n";
• functionality that helps user to write the input parameter std::string_view message; since it isn't C++20 and std::format isn't available... current standard methods aren't very convenient. Though, it doesn't have to be a part of the logger and you can just use, say, the same fmt library. Also you might want to avoid making the message at all if logger is going to filtrate it anyways.
• The functionality that determines whether to print the message: add verbose level, don't just rely on the message type. At times you don't care if a low-level function returns a warning or an error, but if you'd still want to print info from a high level function.
• Current version is not portable. Consider making an interface that is portable, even if the implementation isn't - at most make a different class for other platforms when the time comes. Changing interface will be much harder.
• Logger::get_error_string why return a std::string and not a std::string_view or const char*? Though, it won't matter much if you have short-string-optimization.
• You don't want every method to be in .cpp consider moving quick ones in the header. This way optimizer has more freedom and will be able to inline them if it chooses to. Though, you shouldn't move everything to header as then compilation times will increase.

• Also no need for declaring default constructor, you can just declare default initialization of the class members when you declare the variables

  const HANDLE    m_console_handle    = GetStdHandle(STD_OUTPUT_HANDLE); 
  LogLevel        m_current_log_level = LogLevel::Trace;
  LoggerSettings  m_settings          = {};
  

• About Logger::get_file_name - just use size_t instead of int for index and i to avoid awkward casting. (Though this way you'll have a bug when pathfile is an empty string...) Also, your error/warning configuration is too strict, checkout where you can configure it. It shouldn't be an error but a warning without the casting. Moreover, why not use std::string or std::string_view functions? And this method should be static.

  static std::string_view Logger::get_file_name(std::string_view filepath)
  {
     auto index = filepath.find_last_of('\\');
  
     if(index != std::string::npos)
     {
        return filepath.substr(0,index);
     }
  
     return {};
  }
  

Answer 2

I have a number of suggestions which might help you improve your program.

Rethink the name

The name Logger makes me think of creating a log which is a record of something that can be reviewed later. However, that's not what this does. It emits color coded messages to the console instead, so perhaps Logger is not the best term, since there isn't any record created. Maybe Tracer would be a better name.

Fix the bug

There is an error in the constructor of LoggerSettings:

print_level(should_print_color),

I'm betting that should be should_print_level as the intializer.

Avoid function-like macros

In modern C++, there is not much use any more for function-like #define macros. They lack type checking and are prone to error, so I'd avoid using them. In this particular case, they can't really work anyway since they don't allow for a way to specify a particular instance of the Logger class. Bite the bullet and just use __FILE__ and __LINE__ explicitly as needed. See ES.31 for details.

Use const where practical

The get_settings() function doesn't and shouldn't alter the underlying Logger class instance, so it should be declared const. One could make the same argument for the various log functions.

Make sure you have all required #includes

The code uses std::string_view but doesn't #include <string_view>. It should.

Use classes to better advantage

If you defined a Color class, this code might be simplified by relegating color handling functions and data to just that one class. Also, the LoggerSettings class seems rather pointless. I'd just put those member variables directly into the Logger class.

Don't Repeat Yourself (DRY)

There is a separate function for each log level and a corresponding color function for each. That is a lot of repeated code. What I'd suggest is that you create a class within the Logger.cpp file something like this:

struct LogLevelData {
    LogLevel level;
    std::string_view name;
    Color color;
    void log(bool print_color, bool print_level, bool print_file_and_line, const std::string_view message, const char* file, int line) const;
};

static constexpr LogLevelData logdata[5]{
    { LogLevel::Trace, "Trace", BackgroundColor::Black & ForegroundColor::Intense_white },
    { LogLevel::Info, "Info", BackgroundColor::Black & ForegroundColor::Intense_green},
    { LogLevel::Warn, "Warn", BackgroundColor::Black & ForegroundColor::Intense_red},
    { LogLevel::Error, "Error", BackgroundColor::Black & ForegroundColor::Intense_Cyan},
    { LogLevel::Fatal, "Fatal", BackgroundColor::Black & ForegroundColor::Intense_magenta},
};

Now we can delegate most of the work and eliminate duplication:

void Logger::log(const LogLevel level, const std::string_view message, const char* file, int line) {
    if (level >= m_current_log_level) {
        logdata[static_cast<int>(level)].log(print_color, print_level, print_file_and_line, message, file, line);
    }
}

Use include guards

There should be an include guard in each .h file. That is, start the file with:

#ifndef LOGGER_H
#define LOGGER_H
// file contents go here
#endif // LOGGER_H

The use of #pragma once is a common extension, but it's not in the standard and thus represents at least a potential portability problem. See SF.8

Streamline the interface

One way to rewrite your Logger.hpp file is like this:

#ifndef LOGGER_H
#define LOGGER_H
#include <string_view>

namespace AGE
{
    enum class LogLevel
    {
        Trace,
        Info,
        Warn,
        Error,
        Fatal
    };

    class Logger
    {
    public:
        void set_log_level(const LogLevel level);
        void log(const LogLevel level, const std::string_view message, const char* file, int line);

    private:
        LogLevel m_current_log_level = LogLevel::Trace;
        bool print_level = true;
        bool print_file_and_line = true;
        bool print_color = true;
        bool print_time = true;
    };
}
#endif // LOGGER_H

Note that there is no longer any need for an explicit constructor, any macros, references to anything in Windows.h or the LoggerSettings class. It's useful to try for a minimal but sufficient interface that hides as many of the implementation details as practical.

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. Here's the code I eventually used to exercise a rewrite of your class:

#include "Logger.hpp"
int main() {
    AGE::Logger log;
    log.log(AGE::LogLevel::Trace, "The merest trace", __FILE__, __LINE__);
    log.log(AGE::LogLevel::Info, "Some information", __FILE__, __LINE__);
    log.log(AGE::LogLevel::Warn, "A warning", __FILE__, __LINE__);
    log.log(AGE::LogLevel::Error, "This is an error", __FILE__, __LINE__);
    log.log(AGE::LogLevel::Fatal, "Catastrophe!", __FILE__, __LINE__);
}

Consider the user

The colors actually used are, of course, up to you, but I'd suggest that using red for a warning and magenta for a fatal error is rather counterintuitive. Red is traditionally used for the thing that means "stop" and in this case, I'd say that's probably a fatal error.

Be consistent with capitalization

Of the foreground color list, only Intense_Cyan has the second word capitalized. It's a minor point but inconsistency in the interface is annoying to anyone who later uses this code.

Answer 3

One more thing I'd want to add. I don't think you want to use the whole (__FILE__, __LINE__) mess.

In a small codebase, you should be able to easily find the file and line via a simple search while in large codebase file/line info is just not sufficient. Say, you have a file used in many places, then printing that an error occurs in this file is not very informative - you'd want the whole callstack or at least some partial ownership information.

So consider adding a second layer over the logger class - one that classes tend to keep a copy of. One that keeps a pointer to the interface; provides the functionality for composing the message from multitude of data; filtrates on-the-root messages that aren't going to be printed; and keeps an "owner" or "callstack" string and automatically adds it to the message so you can identify who actually prints the message, and not just file/line info.