This code is incomplete, so we'll have to guess there's a definition of namespace DayOfYear
, or struct DayOfYear
, somewhere. In future, please provide more context for reviews - ideally, we'd be able to take your code and compile it ourselves.
There's a problem with the interface: to convert from Julian day number to month, we need to know whether the year is a leap year or not. So convertDayOfYear()
needs two arguments - as well as the day number, we need to provide either the year number or an indication (as a bool
) whether it's a leap year. From C++20, it's straightforward to convert, using std::chrono::year::is_leap()
. We can then use that information like this:
if (!is_leap_year && num >= 59) { // day 59 is 1st of March
num += 1;
}
We want to reduce that big chain of repeated if
/else
. One thing we could do to simplify is to not repeat the previous test: if we're in the else
block from if (num <= 31)
, we already know that num > 31
:
if (num <= 31)
return months[0] + " " + to_string(num);
else if (num <= 59)
return months[1] + " " + to_string(num - 31);
else if (num <= 90)
What we'd really like is to be able to just look it up in a table. We can do this using a list of months with their day counts:
const std::vector<std::pair<int, std::string>> day_to_month = {
{ 31, "January" },
{ 29, "February" }, // we'll correct non-leap years to fit
{ 31, "March" },
{ 30, "April" },
{ 31, "May" },
{ 30, "June" },
{ 31, "July" },
{ 31, "August" },
{ 30, "September" },
{ 31, "October" },
{ 30, "November" },
{ 31, "December" },
};
We can walk this list, and at each element look to see if that month contains the day - if it does, create the return value; if not, then subtract that month's amount of days and move on to the next entry:
for (auto const& entry: day_to_month) {
auto& days_in_month = entry.first;
auto& month_name = entry.second;
if (num <= days_in_month) {
return month_name + " " + std::to_string(num);
} else {
num -= days_in_month;
}
}
I've named the elements of each entry to make it clearer; we could just use entry.first
and entry.second
instead, but I believe the clarity helps here.
Let's put it all together, with some reasonable range checking and a main()
to demonstrate that it works. Let's also change the vector
to be an array
, since we know its length in advance.
Then we get:
#include <array>
#include <stdexcept>
#include <string>
#include <utility>
std::string convertDayOfYear(int num, bool is_leap_year)
{
if (num <= 0)
throw std::range_error("before beginning of year");
// correct non-leap years
if (!is_leap_year && num > 59) { // day 59 is 28th of February
num += 1;
}
static const std::array<std::pair<int, std::string>, 12> days_in_month = { {
{ 31, "January" },
{ 29, "February" }, // we've adjusted non-leap years
{ 31, "March" },
{ 30, "April" },
{ 31, "May" },
{ 30, "June" },
{ 31, "July" },
{ 31, "August" },
{ 30, "September" },
{ 31, "October" },
{ 30, "November" },
{ 31, "December" },
} };
for (auto const& entry: days_in_month) {
auto& days_in_month = entry.first;
auto& month_name = entry.second;
if (num <= days_in_month) {
return month_name + " " + std::to_string(num);
} else {
num -= days_in_month;
}
}
throw std::range_error("after end of year");
}
#include <iostream>
int main()
{
std::cout << convertDayOfYear(1, false) << '\n'
<< convertDayOfYear(1, true) << '\n'
<< convertDayOfYear(59, false) << '\n'
<< convertDayOfYear(59, true) << '\n'
<< convertDayOfYear(60, false) << '\n'
<< convertDayOfYear(60, true) << '\n'
<< convertDayOfYear(365, false) << '\n'
<< convertDayOfYear(366, true) << '\n';
}
If performance is important, then consider using std::lower_bound
to find the month from the list - you'd sacrifice some clarity there, as you'd need to return to using the cumulative day count for each month rather than days in that month (or write code to build the former from the latter).
It's a little bit tricky to get right (both the conversion to cumulative day count and the lookup) but with the help of the test cases, I get this implementation (C++14, because of the auto
parameters in the lambda expression, but easily adapted to C++11):
#include <algorithm>
#include <array>
#include <cassert>
#include <stdexcept>
#include <string>
#include <utility>
std::string convertDayOfYear(int num, bool is_leap_year)
{
if (num <= 0)
throw std::range_error("before beginning of year");
// correct non-leap years
if (!is_leap_year && num > 59) { // day 59 is 28th of February
num += 1;
}
if (num > 366)
throw std::range_error("after end of year");
static const auto start_day = []{
std::array<std::pair<int, std::string>, 12> days_in_month = { {
{ 31, "January" },
{ 29, "February" }, // we've adjusted non-leap years
{ 31, "March" },
{ 30, "April" },
{ 31, "May" },
{ 30, "June" },
{ 31, "July" },
{ 31, "August" },
{ 30, "September" },
{ 31, "October" },
{ 30, "November" },
{ 31, "December" },
} };
int offset = 0;
for (auto& month: days_in_month) {
std::swap(offset, month.first);
offset += month.first;
}
return days_in_month;
}();
// start_day now looks like { {0, "January"}, {31, "February"},
// {60, "March"}, ... } - i.e. the number of days to subtract for
// that month.
// Now search backwards for the first month whose offset is less
// than num
auto it = std::lower_bound(start_day.rbegin(), start_day.rend(), num,
[](auto a, auto b){ return a.first >= b; });
assert(it != start_day.rend());
return it->second + " " + std::to_string(num - it->first);
}
int
can be negative. Best to add a check for that. In real code a lot of time is spent checking for valid inputs, so as well to get into the habit early. \$\endgroup\$