Time duration conversion with units

Question

I recently was asked to upgrade some C++11 code to add a feature by which the user can set a time duration from the command line of the program. The requirement was to have a default time of 1500 ms but to allow it to be overridden by the command line parameters if the parameters are valid; invalid conditions should result in a text message diagnostic, but the program should proceed with the default value. Negative time durations are not valid, so this code negates any value less than zero. The units of time can be ms, s, m, or h for milliseconds, seconds, minutes or hours. The parseDuration code is shown below, along with a simple main to demonstrate its intended use.

#include <chrono>
#include <iostream>
#include <string>
#include <unordered_map>

std::chrono::milliseconds parseDuration(std::chrono::milliseconds& dur, const std::string& num, const std::string& unit) {
    static const std::unordered_map<std::string, long> suffix{ {"ms", 1}, {"s", 1000}, {"m", 60*1000}, {"h", 60*60*1000} };
    try {
        auto n{std::stod(num)};
        if (n < 0) {
            n = -n;
        }
        dur = std::chrono::milliseconds{static_cast<unsigned long>(n * suffix.at(unit))};
    }
    catch (const std::invalid_argument& e) {
        std::cout << "ERROR: could not convert " << num << " to numeric value" << '\n';
    }
    catch (const std::out_of_range& e) {
        std::cout << "ERROR: " << unit << " is not one of ms,s,m,h" << '\n';
    }
    return dur;
}

int main(int argc, char *argv[]) {
    if (argc != 3) {
        std::cerr << "Usage: duration num ms|s|m|h\n";
        return 1;
    }
    auto duration{std::chrono::milliseconds{1500}};
    parseDuration(duration, argv[1], argv[2]);
    std::cout << "That's " << duration.count() << '\n';
}

Answer 1

Avoid defining your own constants

There's no need to use your own constants for the duration of a millisecond, second, minute and so on; just use the right std::chrono types:

static const std::unordered_map<std::string, std::chrono::milliseconds> suffix {
    {"ms", std::chrono::milliseconds{1}},
    {"s", std::chrono::seconds{1}},
    {"m", std::chrono::minutes{1}},
    {"h", std::chrono::hours{1}},
};

It's even simpler in C++14 where you can make use of std::literals::chrono_literals:

using namespace std::literals::chrono_literals;

static const std::unordered_map<std::string, std::chrono::milliseconds> suffix {
    {"ms", 1ms},
    {"s", 1s},
    {"m", 1min},
    {"h", 1h},
};

Now that you store the time as a std::chrono::milliseconds, you have to do much less casting, at least if you parse input as an integer instead of a double:

auto n = std::abs(std::stol(num));
return n * suffix.at(unit);

If you want to support floating point numbers, then you'll have to still cast the result of the multiplication back to milliseconds:

auto n = std::abs(std::stod(num));
return std::chrono::duration_cast<std::chrono::milliseconds>(n * suffix.at(unit));

Support negative durations

I see you try to avoid negative durations, but I don't see why you would want to restrict that. I would just allow negative durations. If the caller wants to ensure it's always a positive number, they can call std::abs() on the result if so desired.

Don't catch exceptions if you can't do anything useful

You are catching exception, and in the body of the catch statements you log a message to std::cerr. However, afterwards you just return default_duration, and the program will continue as if nothing wrong has happened. Now the caller cannot distinguish between the input not being able to be parsed correctly and someone actually writing "1500 ms".

I suggest either not catching the exceptions, and let the caller do that if desired, or if you do want to avoid exceptions leaking out of parseDuration, ideally you would wrap the return type in std::optional instead (but that requires C++17 support). The caller could then do:

auto duration = parseDuration(argv[1], argv[2]).value_or(1500ms);

Note that if the goal is to have a duration of 1500 ms that can be overridden by the command line, what you should write instead is:

int main(int argc, char *argv[]) {
    auto duration = std::chrono::milliseconds(1500); // default

    if (argc == 3) {
        // override specified, try to parse it
        duration = parseDuration(argv[1], argv[2]);
    }

    ...
}

And have parseDuration() throw an exception if the time specified could not be parsed correctly. You can of course still catch the exception and print a nicer error message if so desired.

Making it more generic

What if you want the result in a higher precision? If you want it in nanoseconds instead, you'd have to modify all the places where you wrote std::chrono::milliseconds before. And what if you want to support multiple duration types? Consider turning it into a template:

template <typename Duration = std::chrono::milliseconds>
std::optional<Duration> parseDuration(const std::string& num, const std::string& unit) {
    using namespace std::literals::chrono_literals;
    static const std::unordered_map<std::string, Duration> suffix {...};
    try {
        ...
        return std::chrono::duration_cast<Duration>(...);
    } catch (const std::exception&) {
        return {};
    }
}

Answer 2

Looks pretty good; these comments are mainly nitpicking:

Error messages should be streamed to std::cerr, not std::cout. Whilst that's correctly done in main(), we're using the wrong stream in parseDuration().

I'm not a fan of both modifying the dur parameter and returning it. I would go for accepting a default by value and returning the parsed/default value:

std::chrono::milliseconds parseDuration(const std::string& num, const std::string& unit,
                                        std::chrono::milliseconds& default_duration)

Then it's easier to use:

auto duration = parseDuration(argv[1], argv[2], std::chrono::milliseconds{1500});
std::cout << "That's " << duration.count() << '\n';

I would make suffix map strings to double values, which will eliminate a compiler warning when promoting to multiply with n.

There's no need to name the caught exceptions we don't use.

Plain map might be more (time/space) efficient than unordered map for this small mapping, though the difference will be very small, probably sub-measurable.

---

Modified code

#include <chrono>
#include <iostream>
#include <string>
#include <unordered_map>

std::chrono::milliseconds parseDuration(const std::string& num, const std::string& unit,
                                        const std::chrono::milliseconds& default_duration) {
    static const std::unordered_map<std::string, double> suffix
        { {"ms", 1},
          {"s", 1000},
          {"m", 60*1000},
          {"h", 60*60*1000} };

    try {
        auto n{std::stod(num)};
        if (n < 0) {
            n = -n;
        }
        return std::chrono::milliseconds{static_cast<unsigned long>(n * suffix.at(unit))};
    }
    catch (const std::invalid_argument&) {
        std::cerr << "ERROR: could not convert " << num << " to numeric value\n";
    }
    catch (const std::out_of_range&) {
        std::cerr << "ERROR: " << unit << " is not one of ms,s,m,h\n";
    }
    return default_duration;
}

int main(int argc, char *argv[]) {
    if (argc != 3) {
        std::cerr << "Usage: duration num ms|s|m|h\n";
        return 1;
    }
    auto duration = parseDuration(argv[1], argv[2], std::chrono::milliseconds{1500});
    std::cout << "That's " << duration.count() << '\n';
}