Logger that writes to text file with std::vformat

Question

This class has a Log function that appends text to a log text file. It takes a format string and a variable amount of arguments, much like a printf sort of function. It then writes the formatted string with the interpolated arguments to the file.

I wanted to know if there are any edge cases I might've missed, or if anything can be done to improve this code?

class Logger {
private:
    Logger() = delete;
    Logger(const Logger&) = delete;
public:
    template <class... FormatArgs>
    static void Log(const std::string_view format, FormatArgs&&... args) {
        std::ofstream logfile("log.txt", std::ios::app);
        logfile << std::vformat(format, std::make_format_args(args...)) << std::endl;
    }
};

Edit

Here are some usage examples*.

Logger::Log("This is an example log message");
Logger::Log("This is a log message with {}", "one argument");
Logger::Log("This is a log message with {} {}", 2, "arguments");

Should produce a log.txt text file in the process's working directory with the following contents.

This is an example log message
This is a log message with one argument
This is a log message with 2 arguments

Answer 1

The class seems pointless, since it can't even be instantiated. Why not a free function (in a suitable namespace, perhaps)?

The function itself opens and closes log.txt each time it's called. That's pretty inefficient, and will cause that file to be appended or created uncontrollably wherever the process's working directory happens to be. I'm not sure I'd want any program doing that to my files (and it will be many files if the process frequently changes directory).

It's impossible to test in isolation, as it can't be decoupled from the file-system. We'd want to be able to write to a std::ostringstream in our tests, and we might want to write to the syslog service or equivalent in a daemon program, to take advantage of OS services such as log forwarding.

The template with forwarding reference arguments (FormatArgs&&) fails to std::forward() them when they are used.

There's really no need to flush output using std::endl when we immediately close the file, since that also flushes. Just use plain \n instead. Flushing output would be sensible if we kept the file open, rather than opening for each call.

We probably want to catch std::format_error, and perhaps also std::ios_base::failure and std::bad_alloc, as it's better for logging to fail than to bring down the whole program (unless it's an audit log, I guess). Perhaps the most flexible option is to return a status value that the caller can choose to ignore or to act on.

Perhaps we should be accepting a std::format_string as first argument (which would enable compile-time checking of its validity) rather than relying on run-time exceptions?

---

Modified code

#include <filesystem>
#include <format>
#include <fstream>
#include <string_view>

class Logger
{
    std::ofstream file = {};
    std::ostream& stream;

public:
    explicit Logger(std::ostream& os)
        : stream{os}
    {}
    explicit Logger(std::filesystem::path& filename)
        : file{filename, std::ios::app},
          stream{file}
    {}

    // This one has compile-time checking
    template<typename... Args>
    bool log(std::format_string<Args...> format, Args&&... args)
    {
        try {
            stream << std::format(format, std::forward<Args>(args)...)
                   << std::endl;
            return true;
        } catch (...) {
            return false;
        }
    }

    // Run-time checking only
    template<typename... Args>
    bool log_unchecked(std::string_view format, Args&&... args)
    {
        try {
            stream << std::vformat(format, std::make_format_args(args...))
                   << std::endl;
            return true;
        } catch (...) {
            return false;
        }
    }
};


#include <gtest/gtest.h>
#include <sstream>

TEST(Logger, Log)
{
    auto s = std::ostringstream{};
    auto logger = Logger{s};

    EXPECT_TRUE(logger.log("{} {}!", "Hello", "world", "junk"));
    EXPECT_EQ(s.str(), std::string{"Hello world!\n"});

    // logger.log("{} {}!", "Hello"); // doesn't compile
    EXPECT_FALSE(logger.log_unchecked("{} {}!", "Hello"));
}

---

For an alternative form of error checking, we could pass a flag to choose whether failures during logging should throw or not. This allows us to preserve the exception's type and content, which may be useful. We can use an extra argument to select the run-time-checked version instead of compile-time-checked (or mix and match parts of each change). That looks like:

    enum log_tag{ unchecked };
    enum log_flags{ throw_on_error = 1 };


    // This one has compile-time checking
    template<typename... Args>
    void log(log_flags flags, std::format_string<Args...> format, Args&&... args)
    {
        try {
            stream << std::format(format, std::forward<Args>(args)...)
                   << std::endl;
        } catch (...) {
            if (flags & throw_on_error) {
                throw;
            }
        }
    }
    template<typename... Args>
    void log(std::format_string<Args...> format, Args&&... args)
    {
        log(log_flags{}, format, std::forward<Args>(args)...);
    }

    // Run-time checking only
    template<typename... Args>
    void log(log_tag, log_flags flags, std::string_view format, Args&&... args)
    {
        try {
            stream << std::vformat(format, std::make_format_args(args...))
                   << std::endl;
        } catch (...) {
            if (flags & throw_on_error) {
                throw;
            }
        }
    }
    template<typename... Args>
    void log(log_tag tag, std::string_view format, Args&&... args)
    {
        log(tag, log_flags{}, format, std::forward<Args>(args)...);
    }

Corresponding changes to tests:

    auto s = std::ostringstream{};
    auto logger = Logger{s};

    logger.log("{} {}!", "Hello", "world", "junk");
    EXPECT_EQ(s.str(), std::string{"Hello world!\n"});

    // logger.log("{} {}!", "Hello"); // doesn't compile
    // std::string fmt{"{}!"}; logger.log(fmt, "Hello"); // doesn't compile

    s.str({});
    logger.log(Logger::unchecked, "{} {}!", "Hello");
    EXPECT_EQ(s.str(), "");

    EXPECT_THROW(logger.log(Logger::unchecked, Logger::throw_on_error, "{} {}!", "Hello"),
                 std::exception);

Answer 2

Just use std::format()

There is no point in using std::vformat() here, you can use std::format() directly:

logfile << std::format(format, std::forward<Args>(args...)) << std::endl;

Using std::vformat() would only be useful if Log() itself took a parameter of type std::format_args. Now you are just making the code less efficient than it has to be.

The only problem is that for std::format() to work, format must be a valid std::format_string, not a std::string_view. But that's probably desirable anyway (see Toby's answer).

template <class... Args>
static void Log(const std::format_string<Args...> format, Args&&... args) {
    …
}

Don't open the log file for every log entry

I would expect a class Logger to open the log file once and store the std::ofstream as a member variable, so it doesn't have to reopen the file every time Log() is called.

Missing error handling

The code is missing any form of error handling. What if the log file cannot be opened or a new entry cannot be appended?