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.

• Li Chen: @Zeta it seems vs2017 accept the two colons and compiles lol. Now I have fixed.
• Zeta: Ah, that's fine. I was talking about the variable's name.
• WernerCD: Instead of Magic() maybe use Current Day Epoch (or, more specifically, this post).
• Li Chen: @WernerCD thanks, it's like two judgments(day and year) have to be made and may be influenced by leap year.
• jpmc26: It doesn't have a time zone. This is suicide in practice when working with temporal data. When you get into the real work force, never try to write something like this yourself.

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.

• Li Chen: thanks! But maybe you mean std::ostream &write(std::ostream &dest) const {...}. operator<< can't be member function : )
• Toby Speight: @czxyl, thanks for spotting that - I should have tested!
• Li Chen: Ah, another small problem is static array month_length ranges from 0 to 11 while month above is 1 to 12, I have added 666 as the placeholder to the first element.
• Li Chen: your solution to check_valid quite inspires me a lot and other suggestions adhere me to many aspects which I nerve paid attention to. Thanks for codereview.com and Toby Speight and others who review my codes. HAPPY!
• Toby Speight: I fixed the off-by one, using month-1 to index; I think that's cleaner

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.

• Toby Speight: Good review - I missed the pointless lambda in sorting. I've used make_tuple where you have tie, since the members are primitives and so copying is cheap. Compiler optimisation will likely lead to the same code for both.