Header file for accessing components globally in library

Question

In my project I have a header file which contains only functions which are put into a namespace, the purpose of these functions is to be used from another class of the framework I'm working on, or from client code.

Does this code follow best practices? How can I efficiently just keep such data?

#pragma once

#include <iomanip>
#include <sstream>
#include <chrono>
#include <ctime>

namespace lwlog::datetime
{
    std::string get_chrono(std::string format)
    {
        auto now = std::chrono::system_clock::now();
        auto in_time_t = std::chrono::system_clock::to_time_t(now);

        std::stringstream ss;
        ss << std::put_time(std::localtime(&in_time_t), format.c_str());
        return ss.str();
    }

    std::string get_time()                      { return get_chrono("%H:%M:%S"); }
    std::string get_date()                      { return get_chrono("%Y-%m-%d"); }
    std::string get_date_short()                { return get_chrono("%m/%d/%y"); }
    std::string get_second()                    { return get_chrono("%S"); }
    std::string get_minute()                    { return get_chrono("%M"); }
    std::string get_hour_24()                   { return get_chrono("%H"); }
    std::string get_hour_12()                   { return get_chrono("%I"); }
    std::string get_weekday()                   { return get_chrono("%A"); }
    std::string get_weekday_abbreviated()       { return get_chrono("%a"); }
    std::string get_day()                       { return get_chrono("%d"); }
    std::string get_month()                     { return get_chrono("%m"); }
    std::string get_month_name()                { return get_chrono("%B"); }
    std::string get_month_name_abbreviated()    { return get_chrono("%b"); }
    std::string get_year()                      { return get_chrono("%Y"); }
    std::string get_year_short()                { return get_chrono("%y"); }
}

Answer 1

• As noted by commenters, you will get multiple definition errors if you try to include this header in multiple translation units. The functions must be marked inline, or the header file reduced to the declarations, with the definitions placed in a separate source file.

• The std::string format argument is copied unnecessarily as it is passed by value. It should be passed by const& instead. (Or replaced with std::string_view).

• The difference between all these functions is the format string. As such, we don't really need the functions at all. We can define the format strings as constants, and the user can pass the one they need to get_chrono.

• get_chrono is probably not the best name for the function. Maybe format_time_now, or format_time.

• The function would be much less restrictive if the time were passed as an argument. It can then be used for formatting other times than now.

• Note also that in the original code, if we wanted to call get_second and get_minute for one point in time (say for some custom output format), we can't! Because now will have changed between the calls, and we might end up with incorrect output.

Answer 2

There are many useless functions here, because individual time components aren't much use separately. For example, get_second() is unlikely to be any use to anyone, as users generally want the minutes, too. It won't work to separately call get_minute(), because that returns the "minute" component of a different time-point (that of the second call). So the API presented is likely to encourage user errors.

I advise removing all the functions that return individual components.

There exist single formats for time and date - use %T instead of %H:%M:%S, and %F instead of %Y-%m-%d. (Note that %D superficially appears like it might be the short date formate, but it's broken because it puts month first).

If we're to use this for logging, then std::localtime is a poor choice, as this has jumps and ambiguities around daylight savings changes. std::gmtime is more suitable for this purpose, as it's monotonic on a well-administered system.

Oh, and you'll need your functions to have static linkage, so you don't get conflicts when two or more translation units both define the functions.