Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I want to teach about "encapsulation" and chose Date with Year, Month and Day as an example -- because it demonstrates type-safety w.r.t. preventing accidental swapping of parameters. I want to demonstrate encapusulation to the extreme, meaning I want to hide the int-values of the Year, Month and Day completely and instead define the operations in them as required.

Disregarding if its good to go to this extreme when encapsulating, has anyone comments about my demonstration code?

Intro section

// #!cpp filename=33a-dateplus.cpp
#include <iostream>
#include <iomanip>
using std::ostream; using std::setfill; using std::setw;

Helper value classes

Helper value class Year

class Year {
    int value_; // eg. 2014
public:
    explicit Year(int v) : value_{v} {}
    Year& operator+=(const Year& other) {
        value_ += other.value_;
        return *this;
    }
    friend ostream& operator<<(ostream& os, const Year&x) {
        return os << setfill('0') << setw(4) << x.value_;
    }
    bool isLeap() const;
};

Helper value class Month

class Day;
class Month {
    int value_; // 1..12
public:
    // v may be invalid month-number, to be normalized later, but >0 .
    explicit Month(int v) : value_{v} {}
    Month& operator+=(const Month& other) {
        value_ += other.value_;
        return *this;
    }
    friend ostream& operator<<(ostream& os, const Month&x) {
        return os << setfill('0') << setw(2) << x.value_;
    }
    void normalize(Year &year);
    // precond: month must be normalized; value_ in [1..12]
    Day days(const Year& inYear) const;
    friend bool operator<(const Month &l, const Month& r) {
        return l.value_ < r.value_;
    }
};

Helper value class Day

class Day {
    int value_; // 1..31
public:
    // v may be invalid day-of-month, to be normalized later, but >0 .
    explicit Day(int v) : value_{v} {}
    Day& operator+=(const Day& other) {
        value_ += other.value_;
        return *this;
    }
    Day& operator-=(const Day& other) {
        value_ -= other.value_;
        return *this;
    }
    friend bool operator<(const Day& l, const Day& r) {
        return l.value_ < r.value_;
    }
    void normalize(Month& month, Year& year);
    friend ostream& operator<<(ostream& os, const Day&x) {
        return os << setfill('0') << setw(2) << x.value_;
    }
};

Date, the class we are mainly designing

class Date {
    Year year_;
    Month month_ {1};
    Day day_ {1};
public:
    explicit Date(int y) : year_{y} {} // year-01-01
    Date(Year y, Month m, Day d) : year_{y}, month_{m}, day_{d} {}
    friend ostream& operator<<(ostream& os, const Date&x) {
        return os << x.year_ << "-" << x.month_ << "-"  << x.day_;
    }
    // add an arbitrary number of days to a date; normalizez afterwards
    friend Date operator+(Date date, const Day& day) {
        date.day_ += day;
        date.normalize(); // handle overflows
        return date;
    }
    void normalize();
};

Implementing member functions

bool Year::isLeap() const {
    return ( (value_%4==0) && (value_%100!=0) ) || (value_%400==0);
}

Day Month::days(const Year& inYear) const {
    switch(value_) {
    case 1: case 3: case 5: case 7: case 8: case 10: case 12:
        return Day{31};
    case 4: case 6: case 9: case 11:
        return Day{30};
    case 2:
        return inYear.isLeap() ? Day{29} : Day{28};
    }
    return Day{0}; // invalid value_
}

Normalization functions

void Month::normalize(Year &year) {
    if(12 < value_ || value_ < 1) {
        auto ival = value_-1; // -1: for [1..12] to [0..11]
        year += Year{ ival / 12 };
        value_ = value_ % 12 + 1; // +1: back to [1..12]
    }
}

void Day::normalize(Month& month, Year& year) {
    // normalize month, adjusting year
    month.normalize(year);
    // normalize day; adjusting month and year
    while(month.days(year) < *this) {
        *this -= month.days(year);
        month += Month{1};
        if(Month{12} < month) {
            month = Month{1};
            year += Year{1};
        }
    }
 }

// afterwards contains valid values
void Date::normalize() {
    day_.normalize(month_, year_);
}

A test: main

int main() {
    using std::cout;
    Date d1 { Year{2013}, Month{15}, Day{199} };
    cout << d1 << " = ";
    d1.normalize();
    cout << d1 << "\n";

    for(auto yi : {1898, 1899, 1900, 1901,
            1998, 1999, 2000, 2001, 2002, 2003, 2004}) {
        Date d { Year{yi}, Month{3}, Day{366} };
        cout << d << " = ";
        d.normalize();
        cout << d << "\n";
    }

    for(auto yi : {2011, 2012, 2013, 2014}) {
        Date d { Year{yi}, Month{2}, Day{1} };
        cout << d << " +28d = " << d+Day{28} << "\n";
    }
}

Notes

The code is supposed to follow a "modern" programming style. Which here means:

  • C++11: use of {...} for initialization in most cases, auto
  • type-safety, esp. no evil casts
  • a bit more use of class-instances a values, i.e. "value-semantics"

Output

2013-15-199 = 2014-10-16
1898-03-366 = 1899-03-01
1899-03-366 = 1900-03-01
1900-03-366 = 1901-03-01
1901-03-366 = 1902-03-01
1998-03-366 = 1999-03-01
1999-03-366 = 2000-02-29
2000-03-366 = 2001-03-01
2001-03-366 = 2002-03-01
2002-03-366 = 2003-03-01
2003-03-366 = 2004-02-29
2004-03-366 = 2005-03-01
2011-02-01 +28d = 2011-03-01
2012-02-01 +28d = 2012-02-29
2013-02-01 +28d = 2013-03-01
2014-02-01 +28d = 2014-03-01
share|improve this question
2  
I would avoid naming anything with just the lowercase letter l, as this code does in operator<, due to its common visual similarities with the number 1 or capital letter I. I prefer other letters such as a, b, or short strings such as lhs, rhs, to avoid the potential ambiguities. –  Michael Urman Mar 17 at 1:17
    
@MichaelUrman lhs, rhs it is. –  towi Mar 17 at 8:51
add comment

3 Answers 3

I have recently earned my M.Sc. in Comp.Sci. and one of the things that was my main gripes with any examples given to use during programming classes was the lack of consistency. So I'll say this, please be consistent and if you implement one arithmetic or relational operator you need to implement all of them that make sense.

And show them how to implement arithmetic and relational operators properly, some thing like this:

T operatpr -()  const{
    T(*this) t;
    ...
    return t;
}

T operator += (const T& rhs){
   ...
   return *this;
}

T operator + (const T& rhs) const{
   return T(*this) += rhs;
}

T operator -= (const T& rhs){
    return *this += (-rhs); 
}

T operator - (const T& rhs) const{
    return *this + (-rhs);
}

bool operator < (const T& rhs) const{
    return ...;
}

bool operator > (const T& rhs) const{
    return rhs < *this;
}

bool operator <= (const T& rhs) const{
    return !(*this > rhs);
}

bool operator >= (const T& rhs) const{
    return !(*this < rhs);
}
share|improve this answer
3  
+1 for pleasing, textbook-like correctness; but note that the OP is adding Days to Date (not adding Date to Date); and that if he weren't, IMO Date-and-Date operators should be avoided (I would prefer Date-and-Timespan operations). –  ChrisW Mar 16 at 14:56
1  
Yeah of course, I prefer date+timespan too. Date+date has no semantic meaning as opposed to date+timespan. I just gave example on how to implement all operators based on just a few of them. For the arithmetic operators it's just a matter of swapping T for a timespan and implementing. :) –  Emily L. Mar 16 at 15:02
    
"if you implement one arithmetic or relational operator you need to implement all of them that make sense"... hrm, good point. But as @ChrisW says, with these date-things one needs to take special brain-care "what makes sense". But you are right, I evaded that question by only implementing what I needed. I agree, I should implement more of them: I will add - for most classes. I will implement < and == for all of them. Note that I will "be consistent" by only providing < and == like the stdlib requires at several points. My guess is that one rarely need more -- using the stdlib. –  towi Mar 16 at 19:51
    
True, date+timespan would make sense. I use Day as member in Date as well as a timespan for arithmetics. That's semantically not very nice. I'll mull that over, but I guess I will keep this for my "simple" teaching example. –  towi Mar 16 at 19:53
1  
You are using member functions for all your implementations, i.e. T T::operator+(T& rhs). I would discourage that and would use free functions T operator+(T& lhs, T& rhs). It will allow you to be more consistent when the left and right arguments are different. Note that this is also the case when you have more heavy-weight types and want to offer Move-Semantics with &&-overloads. Then you have to provide implementations for all variants of T&/T&, T&/T&&, T&&/T& and T&&/T&&. That can not be done as member functions. (I left out the consts in the signatures). –  towi Mar 16 at 20:15
show 1 more comment

because it demonstrates type-safety w.r.t. preventing accidental swapping of parameters

It does that: because the constructors which take an int parameter are marked explicit.

has anyone comments about my demonstration code?

I'm unsure why you mark member methods as friend.

Perhaps the Date constructor should implicitly invoke Date::normalize (because I don't like two-stage construction, where user code should remember to invoke normalize on a newly-constructed Date).

Sometimes you pass by const reference e.g. Day& operator+=(const Day& other) and sometimes you pass by value e.g. Date(Year y, Month m, Day d).

Check a good reference book for the right way to define operator+ and operator+=. Instead of ...

friend Date operator+(Date date, const Day& day) {
    date.day_ += day;
    date.normalize(); // handle overflows
    return date;
}

... I suspect that the right way to define it is something like this ...

Date operator+(const Day& day) {
    Date date = *this; // make a copy
    date.day_ += day; // alter the copy
    date.normalize(); // handle overflows
    return date; /// return the copy
}

The comment precond: month must be normalized; value_ in [1..12] implies something tricky or wrong in the public API. Maybe months should always be normalized; if they can't be, maybe this trickery needs to be private and accessible to friend Date (or something like that). Maybe all the normalize methods should be private.

This statement return Day{0}; // invalid value_ should perhaps be a thrown exception. Are you able to construct test/user code which triggers that condition?

Whitespace is unconventional e.g. in ( (value_%4==0) && (value_%100!=0) ) ... I would have expected ((value_ % 4 == 0) && (value_ % 100 != 0)). Maybe your code editor/IDE has a "format document" command to auto-format such things.

Instead of this trickery ...

void Month::normalize(Year &year) {
    if(12 < value_ || value_ < 1) {
        auto ival = value_-1; // -1: for [1..12] to [0..11]
        year += Year{ ival / 12 };
        value_ = value_ % 12 + 1; // +1: back to [1..12]
    }
}

... maybe Month values could be stored internally as 0 .. 11, converted from 1 .. 12 in the constructor, and converted to 1 .. 12 in the stream output. Maybe that would be a good demonstration of encapsulation.

Maybe you should throw if a negative int is passed to a constructor, or use an unsigned int type (though you should perhaps allow negative years, but then again things like the Gregorian calendar change makes early dates meaningless).

Perhaps you should also be able to subtract days from a Date.

share|improve this answer
    
"methods as friend." I am accessing private value_ from a global function. Oh, I see that you meant that operator+() could be a member function; hrm... I'll check. "call Date::normalize in c'tor" -- yes, true. "normalize methods should be private" good point. I guess I did that public for demo-reasons. "Month values 0..11", like other implementations? Yeah well, that I consider a matter of taste, since everything is hidden anyway. "Maybe you should throw" yes, I didn't because I have not covered Exceptions yet. Point very well taken. –  towi Mar 16 at 13:52
    
operators can be defined as members or as free functions but I think there's a preferred way to do it; IIRC there's a chapter ("Item 19") in Effective C++ which suggests they should be members, the rationale being "Whenever you can avoid friend functions, you should, because, much as in real life, friends are often more trouble than they're worth." Maybe not a great rationale, but it does mean that IMO there are more- and less-canonical ways to do it. –  ChrisW Mar 16 at 14:53
    
I concur with the trickery. You shouldn't be able to construct an object with an invalid state so I believe throwing from the constructor should be done. –  Emily L. Mar 16 at 15:08
    
@EmilyL. I totally agree. When I use this example in a later course stage I will be using exceptions. At this specific stage I will keep the "contractional comments". On your both behalf I will add a note at the end about "one should move use exceptions there". –  towi Mar 16 at 19:56
2  
@towi There are some "rules of thumb" in the C++ Operator overloading FAQ on StackOverflow: The Decision between Member and Non-member –  ChrisW Mar 16 at 20:29
show 7 more comments

comments about my demonstration code?

A few notes:

  • try defining Day and Month in terms of unsigned int, not int;

  • Do not use Date{0} as an invalid value.

Code:

Day Month::days(const Year& inYear) const {
    // instead of this:
    // return Day{0}; // invalid value_
    // use this:
    throw std::runtime_error{"invalid month value"};
}

This way, the error cannot be ignored and the code cannot fail silently.

share|improve this answer
    
Because I mix two semantics for the date elements, I'll keep the int. But you are right, it would be better to have a separate "timespan/diffence" type -- which should support signedness, and the date elements should not. You are right Day{0} is bad, but I haven't introduced exceptions yet. –  towi Mar 18 at 10:34
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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