C++ code for a simple single header logging library

Question

I have been working out on C++ code lately just for fun and decided to make a simple logger in it. The code works fine, but it would be great if someone more knowledgeable could point out any mistakes/bad practices I would have carried along in my code. Also was curious on how I can go about writing testcases for the same and future code.

#ifndef ACE_LOGGER_HEADER
#define ACE_LOGGER_HEADER
#include<chrono>
#include<string>
#include<iostream>
#include<fstream>
#include<mutex>
enum LogLevel { FATAL = 4, ERROR = 3, WARN = 2, INFO = 1, DEBUG = 0 };
class Logger
{
 public:
 static void init(LogLevel priority_level = INFO,bool save_to_file = false,bool console_output = true,std::string log_file_path = "");
 static void Fatal(std::string message);
 static void Error(std::string message);
 static void Warn (std::string message);
 static void Info (std::string message);
 static void Debug(std::string message);
 private:
 static bool initialized;
 static bool console_output;
 static bool save_to_file;
 static LogLevel priority_level;
 static std::string log_file_path;
 static std::mutex logger_mutex; 
 Logger(){}
 
 static Logger get_instance();
 static void log(LogLevel log_level,std::string message);
};
#ifdef ACE_LOGGER_IMPLEMENTATION
bool Logger::initialized = false;
bool Logger::console_output = true;
bool Logger::save_to_file = false;
LogLevel Logger::priority_level = LogLevel::INFO;
std::string Logger::log_file_path = "LOG.txt";
std::mutex Logger::logger_mutex;
Logger Logger::get_instance()
{
 static Logger instance;
 return instance;
}
void Logger::init(LogLevel priority_level,bool save_to_file,bool console_output,std::string log_file_path)
{
 if(console_output == false && save_to_file == false)
 {
 //Both console and file outputs disabled. Exiting logger;
 return;
 }
 
 if(save_to_file)
 {
 // Logging to file enabled
 if(log_file_path != "")
 {
 get_instance().log_file_path = log_file_path;
 }
 get_instance().save_to_file = true;
 }
 
 get_instance().console_output = console_output; 
 get_instance().priority_level = priority_level;
 get_instance().initialized = true;
}
void Logger::log(LogLevel log_level,std::string message)
{
 if (log_level >= get_instance().priority_level && get_instance().initialized)
 {
 logger_mutex.lock();
 bool time_format_avail = false;
 typedef std::chrono::system_clock clock;
 auto now = clock::now();
 auto seconds = std::chrono::time_point_cast<std::chrono::seconds>(now);
 auto fraction = now - seconds;
 std::time_t cnow = clock::to_time_t(now);
 auto milliseconds = std::chrono::duration_cast<std::chrono::milliseconds>(fraction);
 char time_str[100];
 if (std::strftime(time_str, sizeof(time_str), "%H:%M:%S:", std::localtime(&cnow)))
 {
 time_format_avail = true;
 }
 if (get_instance().console_output)
 {
 if (time_format_avail)
 {
 std::cout << time_str << milliseconds.count() << " ";
 }
 std::cout << message << std::endl;
 }
 if (get_instance().save_to_file)
 {
 std::ofstream file(log_file_path, std::ios_base::app);
 if (time_format_avail)
 {
 file << time_str << milliseconds.count() << " ";
 }
 file << message << std::endl;
 file.close();
 }
 logger_mutex.unlock();
 }
}
void Logger::Fatal(std::string message)
{
 log(LogLevel::FATAL,"[FATAL]\t"+message);
}
void Logger::Error(std::string message)
{
 log(LogLevel::ERROR,"[ERROR]\t"+message);
}
void Logger::Warn(std::string message)
{
 log(LogLevel::WARN,"[WARN] \t"+message);
}
void Logger::Info(std::string message)
{
 log(LogLevel::INFO,"[INFO] \t"+message);
}
void Logger::Debug(std::string message)
{
 log(LogLevel::DEBUG,"[DEBUG]\t"+message);
}
#endif
#endif

Answer 1

Use enum class

By default, C-like enum put their enumerators in the enclosing namespace. And that's not great at all.

From C++11 on, you can use enum class instead, to scope those enumerators within the enum itself. And you can also take the opportunity to specify the storage type, so that instead of a 4 bytes int, you can specify std::uint8_t.

Singleton, or not

You are mixing static variables, a static instance returned by value, and by-instance static variable access... and that's not how a singleton is done. At all.

You need to pick one of:

• Either static variables.
• Or instance variables with a global instance.

Do beware that in either case you need to think about the static initialization (and destruction) order fiasco.

I would advise going with a proper singleton as it avoids defining static variables separately.

Avoid redundant parameters

At the moment, you can have 4 distinct combinations of save_to_file and log_file_path in init:

• false and an empty path: OK
• true and a non-empty path: OK
• false and a non-empty path: What does that mean?
• true and an empty path: What does that mean?

You've created woes for yourself and your users by allowing nonsensical combinations to slip through.

Instead, just take a log_file_path argument:

• If it's empty, don't log to file.
• If it's non-empty, log to the specified file.

Use Guard Style

Instead of opening an if block at the beginning of the function, indenting the entire content of it, and closing it at the end, it's better to inverse the boolean and exit early:

 if (<not activated>) {
 return;
 }
 // Do the logging

It avoids losing yourself in humpfteens levels of indentation, and makes it clear from the get go that nothing happens if not activated.

Stop opening and closing that file every single damn time

Opening and closing a file are NOT lightweight operations.

Instead of using a std::string log_file_path data member, just use a std::ofstream log_file one: open it during init, and it'll close itself on destruction.

As a bonus, you don't need a save_to_file boolean member either! If the ofstream is not open, << is a no-op, so no test needed.

Time is always available

You are using strftime with a fixed-width formatting pattern, hence you know in advance exactly how many characters it'll format -- and you could assert it for checking in debug that you're right.

As a result, there's no need for the time_format_avail variable: it's always available.

Avoid catenating strings

Your logging code is inefficient -- taking a string parameter even when not logging -- but that's not a reason to add insult to injury and create another string on top.

Your log method already receives the log-level, to test whether to log or not, override operator<< for LogLevel and be done with it.

Allow deferred formatting after the check

Sometimes, formatting the log string is expensive (and doubly so when allocating a string). In those cases, it's nice to have the ability to check whether the result will be used.

The simple way is to offer a IsEnabled(LogLevel level) method.

Starting from C++11, it's possible to leverage lambdas:

template <typename T>
static void LogLambda(LogLevel level, T&& lambda) {
 if (<not activated>) {
 return;
 }
 std::ostringstream stream;
 lambda(static_cast<std::ostream&>(stream));
 log(level, stream.str());
}

Not Thread-Safe, despite the mutex

The most glaring issue is that your init function doesn't lock the mutex. So if one thread calls init while another is logging, who knows what happens...

Another issue is that you read the is_initialized and priority_level variables prior to locking. It's a good idea to do so... but then they need to be atomic (and you can use relaxed loads).

Locked too early

Your mutex is locked too early.

In general, it's best to try and minimize the amount of time for which a mutex is locked. This means calculating as much as possible beforehand, and only locking at the last moment.

In your case, this means formatting the time first and only then locking before printing to console and file.

Logging doesn't go to stdout

By convention, stdout is for application output, which logging isn't.

Instead, logging goes to stderr, which for this purpose is best accessed via the aptly named std::clog.

Note: The main difference between std::cerr and std::clog is that std::clog is NOT tied to other streams and it's not automatically flushed on end-of-lines.

Should you use std::endl?

std::endl has 2 effects:

• It pushes a newline character.
• It flushes the stream.

Flushing is not necessarily wrong for logging, but it's expensive so it bears thinking about it.

My recommendation would be to flush based on the level:

• Fatal and Error flush, as they are important.
• Other levels don't, they'll appear soon enough.

You could even make this parameterizable, so that for debugging a crashing application, one can activate flushing for all logs.

• Acedev003: Have made some edits based on the suggestions put forward here github.com/Acedev003/cpp_utils/blob/main/ace_logger.hpp. Kindly do review them if possible

Answer 2

General Observations:

It isn't clear why you are designing this log as a singleton. This allows only a single instance of the log in the program, what if I need multiple logs?

Should it be okay to run the init() functions multiple times, there is no test in the init() function to see of the object has been initialized previously.

There are no accessor functions so the only way to change the private variables is by calling the init() function. This is non-intuitive.

In the log() method, construct the string only once and then log it to the proper output channel as many times as you need.

Prefer using Statements Over typedef

Rather than create a new type using typedef you can achieve the same functionality with the using statement.

void Logger::log(LogLevel log_level, std::string message)
{
 if (log_level >= get_instance().priority_level && get_instance().initialized)
 {
 logger_mutex.lock();
 bool time_format_avail = false;
 using clock = std::chrono::system_clock;
 ...

This is more idiomatic in C++.

Constructor versus init() Function

If the class were not designed as a singlton initialization would be much easier with a constructor. There would also be no need for the get_instance() function.

It isn't completely clear why the code has an init() function rather than using a constructor to achieve the initialization of the class.

class Logger
{
public:
 static void Fatal(std::string message);
 static void Error(std::string message);
 static void Warn(std::string message);
 static void Info(std::string message);
 static void Debug(std::string message);
private:
 bool initialized = false;
 bool console_output = true;
 bool save_to_file = false;
 LogLevel priority_level = LogLevel::INFO;
 std::string log_file_path{};
 static std::mutex logger_mutex;
 Logger(LogLevel plevel = LogLevel::INFO, bool saveFile = false, bool useConsole=true, std::string logFile = "")
 : priority_level{plevel}, save_to_file { saveFile }, console_output { useConsole}, log_file_path { logFile}, initialized {true}
 { }
 static Logger get_instance();
 static void log(LogLevel log_level, std::string message);
};

Let the Compiler Do the Work

If the order of the enums in LogLevel was reversed there is no need to specify the value:

enum LogLevel { DEBUG, INFO, WARN, ERROR, FATAL};

This makes the code easier to maintain.

The constants aren't actually necessary since the code never tries to use the numeric value.

Possible Optimization

If you are using any optimization this isn't necessary, since the compiler will do it for you, but in init() there are multiple calls to get_instance() when there really only needs to be one.

void Logger::init(LogLevel priority_level, bool save_to_file, bool console_output, std::string log_file_path)
{
 if (console_output == false && save_to_file == false)
 {
 //Both console and file outputs disabled. Exiting logger;
 return;
 }
 Logger thisLog = get_instance();
 if (save_to_file)
 {
 // Logging to file enabled
 if (log_file_path != "")
 {
 thisLog.log_file_path = log_file_path;
 }
 thisLog.save_to_file = true;
 }
 thisLog.console_output = console_output;
 thisLog.priority_level = priority_level;
 thisLog.initialized = true;
}

Answer 3

That's a single header implementation in name only, but it isn't a header-only implementation. You need one (and exactly one) implementation file that uses some undocumented magic, namely

#define ACE_LOGGER_IMPLEMENTATION
#include "ace_logger.h"

That aside, your std::string arguments should almost all be const std::string& instead.

• Toby Speight: I think the string arguments might be better as std::string_view, avoiding object creation when const char* supplied.
• Zeta: @TobySpeight That would be C++17 though, and the question is tagged c++11. If OP is fine with C++17, then yes, definitely!