A Date Class with simple methods

Question

This is homework from one MOOC(no requirement of forbidding paste codes online). The title gave me a framework:

class Date
{
    int month;
    int day;
    int year;
};

And it asks me to implement operator<(), operator>(), operator==(), as well as print() and sort().

Below codes is my implementation:

/*=============================================================*/

//date.h

#ifndef DATE_H
#define DATE_H
#include <iostream>
class Date
{
    int month;
    int day;
    int year;
    static char colon;
public:
    Date(int m, int d, int y) : month(m), day{d}, year{y} {}
    bool operator>(Date const& another) const { return this->magic() > another.magic(); };
    bool operator<(Date const& another) const { return this->magic() < another.magic(); }
    bool operator==(Date const& another) const { return this->magic() == another.magic(); }
    void print() const { std::cout << year << colon<< month << colon << day << std::endl; }
    long int magic() const { return static_cast<long int>(month * 100 + day + year * 10000); }
};
#endif // !DATE_H


/*=============================================================*/

//date.cpp

#include "stdafx.h"
#include "date.h"
char Date::colon = '-';

/*=============================================================*/

//extras.hh

#ifndef EXTRAS_HH
#define EXTRAS_HH
#include <vector>
#include <ctime>
#include <cstdlib>
#include <iostream>
#include <algorithm>
#include "date.h"
#include "extras.hxx"
inline void description();
inline void checkValid(int month, int day, int year);
inline auto generateDate();
inline void Sort(std::vector<Date>& v);
inline void print(std::vector<Date>& Dates);
#endif


/*=============================================================*/

//extras.hxx
//To use inline and seperate inline functions' declaration and definition

#include "stdafx.h"
#include <iostream>
#include <vector>
#include <ctime>
#include <cstdlib>
#include <iostream>
#include <algorithm>
#include "date.h"
struct InvalidDate {};
inline void description()
{
    std::cout
        << "Request: " << std::endl
        << "  (1): CreatePoints: generates Dates;" << std::endl
        << "  (2): Sort: sort the dates" << std::endl
        << "I will check if dates generated by rand() is valid, such as LeapYear"
        << std::endl
        << std::endl;
}
inline void checkValid(const int month, const int day, const int year) noexcept(false) 
{
    if (day > 31 || month > 12 || day <= 0 || month <= 0 || year < 0) 
    {
        std::cout << month << "/" << day << "/" << year << ": " << "day > 31 || month > 12 || day <= 0 || month <= 0 || year < 0 is not valid? " << std::endl;
        throw InvalidDate();
        return;
    }

    if ((month == 4 || month == 6 || month == 9 || month == 11) && day == 31)
    {
        std::cout << month << "/" << day << "/" << year << ": " << "it is a solar month of 30 days" << std::endl;
        throw InvalidDate();
    }

    if (month == 2)
    {
        if ((year % 100 != 0 && year % 4 == 0) || year % 400 == 0)
        {
            if (day > 29)
            {
                std::cout << month << "/" << day << "/" << year << ": " << "is LeapYear!!" << std::endl;
                throw InvalidDate();
                return;
            }
        }
        else
        {
            if (day > 28)
            {
                std::cout << month << "/" << day << "/" << year << ": " << "is LeapYear!!" << std::endl;
                throw InvalidDate();
                return;
            }
        }
    }
}

inline auto generateDate() noexcept(false) 
{
    std::srand(std::time(0));
    std::vector<Date> ans;
    int month = 0, day =0, year = 0;
    for (auto i = 0; i < 10; i++)
    {
        month = std::rand() % 15, day = std::rand() % 35, year = std::rand() % 10000;
        while (true)
        {
            try
            {
                checkValid(month, day, year);
            }
            catch (InvalidDate)
            {
                month = std::rand() % 15, day = std::rand() % 35, year = std::rand() % 10000;
                continue;
            }
            break;
        }
        ans.push_back(Date( month, day, year ));
    }
    return ans;
}

inline void Sort(std::vector<Date>& v)
{
    std::sort(std::begin(v), std::end(v), [](Date const& a, Date const& b) { return a < b; });
}

inline void print(std::vector<Date>& dates) 
{
    for (auto date : dates) date.print();
}

/*=============================================================*/

//Main.cpp

#include "stdafx.h"
#include <iostream>
#include "extras.hh"
#include "date.h"
int main()
{ 
    description();
    std::vector<Date> dates = generateDate();
    Sort(dates);
    print(dates);
}

Here is a sample output in console

Request:
  (1): CreatePoints: generates Dates;
  (2): Sort: sort the dates
I will check if dates generated by rand() is valid, such as LeapYear

8/0/3339: day > 31 || month > 12 || day <= 0 || month <= 0 || year < 0 is not valid?
14/33/2889: day > 31 || month > 12 || day <= 0 || month <= 0 || year < 0 is not valid?
0/19/3260: day > 31 || month > 12 || day <= 0 || month <= 0 || year < 0 is not valid?
0/12/8402: day > 31 || month > 12 || day <= 0 || month <= 0 || year < 0 is not valid?
778-8-6
1679-12-15
2029-8-28
2614-5-10
4645-6-2
4913-12-3
5253-3-7
8024-4-3
9589-5-26
9632-11-4
Press any key to continue . . .

How can I enhance these codes? Thanks in advance.

Answer 1

What's in the public interface?

Most of the public interface looks reasonable, except it's not obvious what Date::magic() means to a user - I think that's more useful as a private method.

The print() method is inflexible, as it will only write to std::cout - users are out of luck if they want to write to std::cerr or to a std::ostringstream, for instance. Consider instead a more idiomatic print function:

std::ostream& operator<<(std::ostream&, const Date&);

(You'll need to include <iosfwd> to declare std::ostream.)

Range checking should be part of the class

The check_valid() method logically belongs to the Date class; it probably makes sense for it to be called as part of the constructor:

    /* more intuitive order of arguments! */
    Date(int year, int month, int day)
        : month{month}, day{day}, year{year}
    {
        checkValid();
    }

private:
    void checkValid() const
    {
          if (month <= 0 || month > 12)
              throw std::domain_error("month");
          // etc
    }

Use an array of month lengths

Instead of the unwieldy test (month == 4 || month == 6 || month == 9 || month == 11) && day == 31), we could use a table of month lengths. Something like

static const int month_length[] = { 31, 28, 31, 30, 31, 30,
                                    31, 31, 30, 31, 30, 31 };
auto max_day = month_length[month-1];  // we've already checked 0 < month <= 12
if (month == 2 && is_leap_year(year))
    ++max_day;

if (day <= 0 || day > max_day)
    throw std::domain_error("day");

Consider std::tuple comparison

Instead of creating a collation key using the magic() function (which probably wants a better name, but hey, naming is hard), it's possible to compare the fields in turn by combining them as a tuple:

bool operator<(Date const& other) const
{
    return std::make_tuple(year, month, day)
         < std::make_tuple(other.year, other.month, other.day);
}

Consider using std::rel_ops to save writing the other relational operators by hand (you missed <=, >= and !=, for example).

Pass by const reference where possible

The print(vector) implementation can benefit from a couple of const declarations and auto& rather than auto:

inline void print(std::vector<Date> const& dates) 
{
    for (auto const& date: dates)
         date.print();
}

Finally

char Date::colon = '-';

That's the most misleading name I've seen for a while - I suggest field_separator or even just separator as an alternative. Given that it's class scope (i.e. static), it probably ought to be const, or you risk weird bugs at a distance.

Answer 2

And it asks me to implement operator<, operator>, operator==, as well as print and sort dates.

Only going to review these parts.

---

    bool operator>(Date const& another) const { return this->magic() > another.magic(); };
    bool operator<(Date const& another) const { return this->magic() < another.magic(); }
    bool operator==(Date const& another) const { return this->magic() == another.magic(); }
    long int magic() const { return static_cast<long int>(month * 100 + day + year * 10000); }

Should magic be publicly exposed?

Prefixing this-> is implied. Typically used in objects you maintain, so as long as you avoid shadowing names, you shouldn't need to prefix this-> and can reduce clutter.

Don't weigh your Date members for ordering. Lexicographically compare your Date data members. That means you should find the first mismatching data member and then apply the comparison operator. C++11 makes this simple by using std::tie (requires <tuple>) on the data members to create a tuple, then using the comparison operators for tuple objects to lexicographically compare.

Using operator< and operator==, you could implement the remaining 4 relational operators through logic.

 a > b ... b < a
a <= b ... !(b < a)
a >= b ... !(a < b)
a != b ... !(a == b)

The tuple comparison operators are simpler.

  private:
    auto tie() const { return std::tie(year, month, day); }

  public:
    bool operator<(Date const& another) const { return tie() < another.tie(); }
    bool operator>(Date const& another) const { return tie() > another.tie(); }
    bool operator==(Date const& another) const { return tie() == another.tie(); }

---

    void print() const { std::cout << year << colon<< month << colon << day << std::endl; }

Writing std::endl to a stream is exactly equivalent to writing a '\n', followed by a std::flush to a stream. For buffered streams, manual flushing could have a significant cost compared to automatic flushing. If you explicitly would like to flush a buffered stream, then stream std::flush so readers of your code know flushing was intended. If you just need a newline, prefer '\n'.

---

inline void Sort(std::vector<Date>& v)
{
    std::sort(std::begin(v), std::end(v), [](Date const& a, Date const& b) { return a < b; });
}

Your lambda is equivalent to using std::less<Date>(). C++14 introduced transparent operator functors, which simply means the type compared is deduced automatically. With C++14, you may omit Date from std::less<>(). std::sort is nice though and uses operator< by default if a comparator isn't provided.

std::sort(v.begin(), v.end(), [](Date const& a, Date const& b) { return a < b; }); // C++11
std::sort(v.begin(), v.end(), [](auto const& a, auto const& b) { return a < b; }); // C++14
std::sort(v.begin(), v.end(), std::less<Date>()); // C++11
std::sort(v.begin(), v.end(), std::less<>()); // C++14
std::sort(v.begin(), v.end()); // C++98

---

inline void print(std::vector<Date>& dates) 
{
    for (auto date : dates) date.print();
}

Be consistent with your const and reference (&) usage. Did you intend to copy date by value or did you intend to reference? Should the argument be const?

Avoid loops without braces. Braces help make it clear to the reader that there is a block of code present. Braces also help clearly define the boundaries of a sub-block.

Prefer using a separate line for single-line blocks to simplify setting breakpoints in debuggers.