Generic calendar which can hold Mayan, Babylonian, Fantasy, Gregorian calendars

Question

I have created this generic calendar class which I call a TimePeriodGrouping. It can contain a series of interlocking time periods (i.e. years, months, days, hours...). When time is added to one of them, it recalculates to match the proper period. Also, since seconds start at 0 and months start at 1, it also accounts for that. Another feature is the function "getcount" which should transform the 24 hour times into AMPM style times.

The difference between this and a normal calendar is that I want it to be generic. In some video games, there are only 4 months in a year and only 12 hours in a day.

It seems to work fine in my test cases at the bottom. I implement a normal calendar down to the seconds.

The reason I post this here is that it seems to work, but it feels very messy and insecure and I don't know what to do to make this more assured. I've added lots of asserts. But I feel like I haven't covered my bases very well.

#include <vector>
#include <string>
#include <cassert>
#include <iostream>

bool incRange(int low, int high, int x)
{
    return (low <= x) && (x <= high);
}

// this can be used to create a calendar! 
class TimePeriodGrouping
{
    struct Period
    {
        std::string name;
        std::vector<int> period;
        int count;
        int indexBase;

        int getcount(int AMPMDividend)
        {
            int ret = count;
            ret %= 12;
            if (ret == 0)
            {
                ret = 12;
            }
            return ret;
        }
    };

    /////////////////// interface
public:
    std::vector<Period> periods;

    void newperiod_back(std::string a_name, std::vector<int> a_period, int a_count, int a_indexBase)
    {
        // assert the next highest has only one period
        assert((periods.back().period.size() == 1));
        // assert that the amount of periods are equal to the size of the period of the next highest period, or 1.
        assert((a_period.size() == periods.back().period.size()) || (a_period.size() == 1));
        // its all good. so add it to the back.
        periods.emplace_back(TimePeriodGrouping::Period{ a_name, a_period, a_count, a_indexBase });
        // make sure the count is not lower than the index base....
        periods.back().count += periods.back().indexBase;
    }

    void add(int i, int addend)
    {
        assert(incRange(0, periods.size()-1, i)); // test index
        periods[i].count += addend;
        recalculate(i);
    }

    void recalculate(int index) // only have to calculate from index to greatest
    {
        for (size_t i = index; i > 0; i--) // Must go from least period and end on the greatest period. Don't run the greatest period.
        {
            int periodSize{ 0 };
            if (periods[i].period.size() > 1) // if the period has more than one period i.e. it is like days which are dependant on which month it is for their period.
            {
                periodSize = periods[i].period[i - 1]; // get the amount of days in that month ( [i-1] cause accessing next higher tier of periods)
            }
            else
            {
                periodSize = periods[i].period.front(); // only has one period, so just use the front.
            }
            // so at 24 hours, because we are at base index of 0. it should roll over to an extra day and 0 hours.
            // calculate if count is over periodSize.
            if (periods[i].count >= periodSize + periods[i].indexBase)
            {
                int carry = periods[i].count / periodSize; // calculate carry
                periods[i].count = periods[i].count % periodSize; // mutate
                periods[i - 1].count += carry; // carry to next higher period
            }
        }
    }

    // Default construct with a base period.
    TimePeriodGrouping(std::string a_name, int a_count, int a_indexBase)
    {
        periods.emplace_back(TimePeriodGrouping::Period{ a_name, {0}, a_count, a_indexBase });
    }
};
////////////////////////////////// TESTING
void test_TimePeriodGrouping()
{
    std::cout << "running: " << __func__ << "\n";
    TimePeriodGrouping tpg{ "Year", 2000, 1 };
    tpg.newperiod_back( "Month", { 12 }, 0, 1 );
    tpg.newperiod_back( "Day", { 30 }, 0, 1 );
    tpg.newperiod_back( "Hours", { 24 }, 0, 0 );
    tpg.newperiod_back("Minutes", { 60 }, 0, 0);

    for (auto& p : tpg.periods)
    {
        std::cout << p.name << " " << p.count << " - ";
    }
    std::cout << '\n';
    tpg.add(0, 19);
    tpg.add(1, 6);
    tpg.add(2, 4);
    tpg.add(3, 13);
    tpg.add(4, 54);
    for (auto& p : tpg.periods)
    {
        std::cout << p.name << " " << p.count << " - ";
    }
    std::cout << '\n';

    tpg.add(0, 0);
    tpg.add(1, 5);
    tpg.add(2, 25);
    tpg.add(3, 10);
    tpg.add(4, 5);
    for (auto& p : tpg.periods)
    {
        std::cout << p.name << " " << p.count << " - ";
    }
    std::cout << '\n';

    tpg.add(4, 1);
    for (auto& p : tpg.periods)
    {
        std::cout << p.name << " " << p.count << " - ";
    }
    std::cout << '\n';
}




int main()
{
    test_TimePeriodGrouping();
}

Answer 1

Wrap your code into a namespace

---

// this can be used to create a calendar! 
class TimePeriodGrouping

Why not name the class something like CustomCalendar to indicate that it is a calendar but not a conventional one?

---

Order your interface from public to private.

---

Naming your member variable period when your surrounding struct is named Period seems like a poor choice. count and name are more bad naming choices. What does this count? What does it name? I'm not sure about indexBase either. Things get even worse later on with things like a_name.
In short, work on your naming.

---

Your comments are more confusing than helping. E.g.

// if the period has more than one period

which in part simply stems from your poor naming choices.

---

AMPMDividend is unused.

---

getcount does not do what its name implies. It either returns count mod 12 or 12 (which is a magic number and should be made into a named constant). From your question it becomes clear it's actually supposed to convert between 24h and AM/PM style. Again, naming.

---

Prefer prefix over postfix

---

Missing <cstddef> and std:: for size_t.

---

When looping with a ranged for loop add const unless you plan to modify the loop variable in the body.

---

Something like this would profit from having unit tests. Maybe even do TDD where you develop the tests first and then write code to pass those tests.

---

You never clearly state a use case. You briefly mention video games but never make clear if this is intended for one (yours or video game developers in general).

---

It would also be nice if you could create a functional calendar from the constructor instead of having to repeatedly call functions.

---

[...]it feels very messy and insecure[...]

This is a good summary of the code.

Date related code, much like crypto, is notoriously hard to get right. Maybe you should take a look at existing solutions (like Boost) and see if you can modify them to suit your use case.