1
\$\begingroup\$

Take an integer representing a day of the year and translate it to a string consisting of the month followed by day of the month. For example, Day 32 would be February 1.

This is what I came up with. I'm sure that there's something more streamlined than else if statements.

vector<string>DayOfYear::months = { "January","February","March","April",
                                "May","June","July","August",
                                "September","October","November","December"};

string DayOfYear::convertDayOfYear(int num){
if (num <= 31)
    return months[0] + " " + to_string(num);
else if (num > 31 && num <= 59)
    return months[1] + " " + to_string(num - 31);
else if (num > 59 && num <= 90)
    return months[2] + " " + to_string(num - 59);
else if (num > 90 && num <= 120)
    return months[3] + " " + to_string(num - 90);
else if (num > 120 && num <= 151)
    return months[4] + " " + to_string(num - 120);
else if (num > 151 && num <= 181)
    return months[5] + " " + to_string(num - 151);
else if (num > 181 && num <= 212)
    return months[6] + " " + to_string(num - 181);
else if (num > 212 && num <= 243)
    return months[7] + " " + to_string(num - 212);
else if (num > 243 && num <= 273)
    return months[8] + " " + to_string(num - 243);
else if (num > 273 && num <= 304)
    return months[9] + " " + to_string(num - 273);
else if (num > 304 && num <= 334)
    return months[10] + " " + to_string(num - 304);
else if (num > 334 && num <= 365)
    return months[11] + " " + to_string(num - 334);
else
    return "Invalid number.\n";
}
\$\endgroup\$
  • 3
    \$\begingroup\$ What about leap years? February can have 28 or 29 days. \$\endgroup\$ – Martin R Nov 1 '18 at 14:26
  • \$\begingroup\$ An 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\$ – rossum Nov 1 '18 at 16:15
  • \$\begingroup\$ Welcome to Code Review! Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ – Toby Speight Nov 2 '18 at 8:54
  • \$\begingroup\$ I recommend that you take your second attempt, and use it as a new question instead. There's certainly some problems with it that you'll want to know about. \$\endgroup\$ – Toby Speight Nov 2 '18 at 8:55
  • \$\begingroup\$ Could you take a look at it here \$\endgroup\$ – dino2018 Nov 2 '18 at 14:07
5
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ Instead of your loop, you can use std::lower_bound if you change the day's part in pairs by the number of days from 1rst january to the first day of the month. \$\endgroup\$ – Calak Nov 2 '18 at 4:32
  • \$\begingroup\$ That's true (and I considered it) - the downside is that it's harder to visually confirm the numbers are correct, so I went for the slower but clearer option. \$\endgroup\$ – Toby Speight Nov 2 '18 at 7:58
  • \$\begingroup\$ Alright, but for completeness, you can maybe edit your code to add it (or I can) \$\endgroup\$ – Calak Nov 2 '18 at 8:13
  • 1
    \$\begingroup\$ Okay, I've added that. I worry it might be beyond OP's level, though. \$\endgroup\$ – Toby Speight Nov 2 '18 at 8:53
  • \$\begingroup\$ it's clean and self explanatory, don't worry about that. If he doesn't understand he can ask and we explain it. \$\endgroup\$ – Calak Nov 4 '18 at 19:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.