6
\$\begingroup\$

This is a Date class in C++. I didn't use C++ strings because I'm not sure if I'm allowed to. It seems to work fine, but I'm wondering if there are any errors I didn't detect. Note that I won't be using this class on its own, it'll be part of another class (it'll serve as a birthdate or something for a person class).

#pragma warning(disable:4996)

#include <iostream>
#include <cstdlib>
#include <cctype>

using namespace std;

class Date {
    private:
        int day;
        int month;
        int year;
    public:
        Date() {
            day = month = year = 0;
        }
        Date(int day, int month, int year) {
            this->day = day;
            this->month = month;
            this->year = year;

        }

        void read_date(void) {
            char date1[11];
            cout << "Enter a date in the form of dd/mm/yy\n";
            cin.getline(date1, 11);
            unsigned length = strlen(date1);
            for (unsigned i = 0; i < length; ++i) {
                if (isalpha(date1[i])) {
                    cerr << "Error: Alphabet character found: " << date1[i] << endl;
                    return;
                }
                if ((ispunct(date1[i])) && (date1[i] != '/')) {
                    cerr << "Illegal character: " << date1[i] << endl;
                    return;
                }
            }
            convert_date(date1);
        }

        void convert_date(char *date1){
            char *result;  
            result = strtok(date1, "/");
            day = strtol(result, nullptr, 10);
            result = strtok(nullptr, "/");
            month = strtol(result, nullptr, 10);
            result = strtok(nullptr, "/");
            year = strtol(result, nullptr, 10);

            if ((day <= 0) || (day >= 32)) {
                cerr << "Day is limited between 1 and 31, you entered: " << day << endl;
                day = month = year = 0;
                return;
            }
            if ((month <= 0) || (month >= 13)) {
                cerr << "Month is limited between 1 and 12, you entered: " << month << endl;
                day = month = year = 0;
                return;
            }
            if ((year <= 1899) || (year >= 2018)) {
                cerr << "year is limited between 1900 and 2017, you entered: " << year << endl;
                day = month = year = 0;
                return;
            }
        }

        void print_date(void) {
            cout << day << "/" << month << "/" << year << endl;
        }

        int get_day(void) {
            return day;
        }
        int get_month(void) {
            return month;
        }
        int get_year(void) {
            return year;
        }
        void set_day(int day) {
            if ((day <= 0) || (day >= 32)) {
                cerr << "Day is limited between 1 and 31, you entered: " << day << endl;
                return;
            }
            this->day = day;
        }
        void set_month(int month) {
            if ((month <= 0) || (month >= 13)) {
                cerr << "Month is limited between 1 and 12, you entered: " << month << endl;
                return;
            }
            this->month = month;
        }
        void set_year(int year) {
            if ((year <= 1899) || (year >= 2018)) {
                cerr << "year is limited between 1900 and 2017, you entered: " << year << endl;
                return;
            }
            this->year = year;
        }
};

int main(void) {
    Date t;
    t.read_date();

    t.print_date();

    t.set_day(1);
    t.set_month(2);
    t.set_year(1950);

    t.print_date();

    while (1);
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Why would you not be allowed to use C++ strings? They are so much better than manipulating memory and character arrays. \$\endgroup\$ – Roland Illig Dec 10 '17 at 15:42
  • \$\begingroup\$ College went straight into classes without going thru vectors/strings. \$\endgroup\$ – user127566 Dec 10 '17 at 15:46
  • \$\begingroup\$ Are you only interested in errors/bugs? There's much more to say about your code. \$\endgroup\$ – Ben Steffan Dec 10 '17 at 15:48
  • \$\begingroup\$ @BenSteffan Mainly errors and bugs, unless there's something that's not an error but still extremely unfavorable. \$\endgroup\$ – user127566 Dec 10 '17 at 15:58
2
\$\begingroup\$

got a few improvement suggestions if you're interested:

  • the sequence of strtok and strtol in convert date can be exchanged with a single call to sscanf. I think sscanf is closer to what you are trying to achieve.

  • it bothers be the default constructor sets an invalid date. either set a valid date (1900-01-01) or remove default constructor all together. setting an invalid date when convert_date fails falls into the same category.

\$\endgroup\$
  • \$\begingroup\$ So basically remove the strtok and strtol parts and replace it with sscanf(date1, "%i/%i/%i", &day, &month, &year);? \$\endgroup\$ – user127566 Dec 10 '17 at 18:37
  • \$\begingroup\$ @MichaelX exactly \$\endgroup\$ – Andreas Dec 10 '17 at 19:51
  • \$\begingroup\$ Or replace them with C++ stream operators >> \$\endgroup\$ – Martin York Dec 11 '17 at 16:18
10
\$\begingroup\$

Not bulletproof

Maximum amount of days in a month depends on both month and a year. It doesn't seem like the class cares about it. The rest of the answer might not be interesting to you.

Operations on an object must be atomic

What I mean by operation on object is getting the object from one valid state to another. Your default constructor violates this. Setting a valid date, for example 1/1/1890 is preferable. Any operation should transition from one valid state to another, or throw exception if it ends up in invalid one. Though sometimes people try to perform invalid operation, which is a different problem altogether.

Not intuitive interface

One doesn't need an object of type Date to convert a string into Date. A free function would be better:

Date to_date(const std::string& datestr);

Easy to use incorrectly

One of the nasty things about dates is that maximum amount of days in a current month is identified by a combination of both year and month. As a result, setting one should trigger a check of others. But when should check be performed? After the set? After which? It is very easy to end up in invalid state.

The solution would be to always accept all of them. Otherwise interface would be terribly bloated.


Order. If I'm not mistaken, people from US are used to month/day/year. Compiler cannot differentiate between the three, and neither do presented class. Ideal would be

auto birth_date = day{21}/month{2}/year{1882};

or at least

date birth_date{day{21}, month{2}, year{1882}};

so that compiler would suggest them correct order and prevent incorrect one.

The former usage syntax is very hard to implement, but the latter is simple:

class date;

class day
{
    friend date;
    unsigned int day;
public:
    day(unsigned int day): day{day} {}
    //cannot do anything else
};

class month
{
    //the same
};

class year
{
    //again
};

class date
{
    /*...*/
};

Maybe you can guarantee that you won't mess up the order, but I don't think the same can be said about people that would maintain your code.


Overall the code seems to be more in C style. My vision of C interfaces is "Here is a bunch of operations. Do whatever you want, I don't care." I apologize if my vision is bluntly wrong, but it is my experience of using C interfaces.

\$\endgroup\$
  • \$\begingroup\$ "One of the nasty things about dates is that maximum amount of days in a current month is identified by a combination of both year and month." – And, even worse, geographic location, since different countries use(d) different calendars at different times. \$\endgroup\$ – Jörg W Mittag Dec 11 '17 at 10:49
  • 1
    \$\begingroup\$ @JörgWMittag It seems reasonable to restrict the class to the Gregorian calendar, in which case the location shouldn’t matter for the purpose of valid dates — or am I mistaken? \$\endgroup\$ – Konrad Rudolph Dec 11 '17 at 12:18
  • 2
    \$\begingroup\$ It depends. At least some people in North Africa and the Arabian peninsula would consider today's date to be 1439-03-22 instead of 2017-12-11, and the current month to have 30 days instead of 31. Really, it's a question of "what do the requirements say"? (Which is pretty much the answer, or the question, to almost anything in programming :-D ) \$\endgroup\$ – Jörg W Mittag Dec 11 '17 at 13:46
  • \$\begingroup\$ What do you mean by auto birth_date = day{21}/month{2}/year{1882};? Is this an overload of the / operator? \$\endgroup\$ – jure Dec 11 '17 at 22:51
  • \$\begingroup\$ @jure, yes, it is \$\endgroup\$ – Incomputable Dec 12 '17 at 3:06
7
\$\begingroup\$

Creating a proper date class is actually fairly complicated, and depends how far you're willing to go to make an interface that adheres to the pit of success principle.

As Scott Meyers warns, adjacent parameters of the same type in an interface are a danger as it may make an interface easy to use incorrectly, the opposite of what you want.

So, one option is to create an enum class month and populate it with the 12 months, so that your constructor can only take these. Then promote the year and day to classes.

Another option is to create something like,

class month{
public:
    static const month jan;
    static const month feb;
    // and so forth
};

which will do just as well as the enum class. You can have your day and year classes restrict what integers you can pass, e.g. day(71) is not allowed.

The problem is, the allowed days in a month changes based on the year. Thus, ideally, you would have a year class, which can serve up the appropriate month objects which restrict in turn the number of days, so that you will always create a valid date.

You can see how things get complicated. It's all about time versus reward: do you need your interface to have these capabilities for your assignment?

\$\endgroup\$
5
\$\begingroup\$

Design

You use three values to represent a date Year/Month/Day. This makes nearly all operations on the class complicated. The only simple operation is printing.

Since you have designed your interface around this structure it may be enough.

But I find the interface very lacking in what I would want to do (add arbitrary periods to the date, compare dates, etc). I think you will find most date classes will convert this into a single value and perform the operations on that value. Then most other operations will becomes simple and printing becomes complex. A single complex operation that is not used often (in comparison to function call count of other functions) would seems a better design.

But since you have not specified a clear use case for the date design sure it may be enough.

Error Handling

Come on this is obviously wrong:

        if ((day <= 0) || (day >= 32)) {
            cerr << "Day is limited between 1 and 31, you entered: " << day << endl;
            day = month = year = 0;
            return;
        }
        if ((month <= 0) || (month >= 13)) {
            cerr << "Month is limited between 1 and 12, you entered: " << month << endl;
            day = month = year = 0;
            return;
        }
        if ((year <= 1899) || (year >= 2018)) {
            cerr << "year is limited between 1900 and 2017, you entered: " << year << endl;
            day = month = year = 0;
            return;
        }

Days and Month cannot be 0. Month cannot be 13 and days cannot be 32 (unless you are living on Pandora). Might be nice if these values were actually specified via a config file? That does make leap years hard to solve though. Also years are wrong based on the messages.

Your code contradicts your error messages. This is absolutely unacceptable. As a maintainer which do I believe the code or the English. Which do I correct? Where is the link to the documentation that I need to understand the code so it can be fixed?

You don't even handle leap years.

 bool isLeapYear(int year) {
     if (year < -46) // 46 BC
         return false;
     }
     if (year < 1582) // Julian Calander
         return year / 4 == 0;
     }
     if ((year >= 1582 and year <= 1923) && checkLocalIsUsingJulian(year)) // Gregorian  Calendar not fully standard until 1923
         return year / 4 == 0;
     }
     // Gregorian  Calendar
     // You may just want to assume a Gregorian calendar (most people do).
     return (year / 400 == 0) || ((year / 4 == 0) && (year / 100 != 0));
 }

Reading Input

The use of strtok() is not a good choice for code as it modifies the input parameters. scanf() is slightly better (and I could use that). But since this is C++ code I think it would be more obvious to use operator>> to read values from a stream.

void convert_date(char *date1){
    char sep1;
    char sep2;
    char errorCheck;

    std::stringstream data1stream(data);
    if (  (data1stream >> d >> sep1 >> m >> sep2 >> y) // Read Data
       && (sep1 == '/' && sep2 == '/')                 // Correct seporators.
       && !(data1stream >> errorCheck))                // No junk at end of input.
    {
         // correctly read and formatted input
    }
    else
    {
         // Error
    }

Code Review

There is a reason for these warnings:

#pragma warning(disable:4996)

You should not disable them. You should fix this warning. The warning is there because you are using outdated functions. Update your code to use the more modern versions.

Don't do this:

using namespace std;

Read this: Why is “using namespace std” considered bad practice?; best answer is here.

Constructor:

        Date() {
            day = month = year = 0;
        }

This is a bad default. Especially since it is an illegal date. Why not default to today?

Initializer List

Prefer to use the initializer list (yes I know it will have no difference on this code). But when you start using classes it will have a difference. You want your code to look consistent no matter what the type is so using the initializer list will always give you the same code and always by the optimal way to do it.

        Date(int day, int month, int year) {
            this->day = day;
            this->month = month;
            this->year = year;    
        }
        // Prefer this:
        Date(int day, int month, int year)
            : day(day)
            , month(month)
            , year(year)    
        {}

Never use this->

Using this-> will hide shadowing errors.

The only reason to use this-> is to deference a variable that is more locally scoped than a member variable. But the problem here is that if you forget then there is no compiler warning to diagnose it (your code just has a bug).

A better approach is to never have shadowed variables. Then you will never need to use this-> to differentiate between the two and it will always be correct. You can also get the compiler to error when there is a shadowed variable.

Learn to use std::string

You have C-Strings splashed around your code. These are very inefficient to use (in comparison to std::string as you have to compute their length all the time). But additionally they are error prone because you need to manually set the length of the underlying input buffer being used.

            char date1[11];
            cin.getline(date1, 11);

That works. But what if I change the size of the buffer? Now I have to search the application for all instances of 11 to make sure all the places it is used are updated correctly.

Now we have to find the actual length:

            unsigned length = strlen(date1);

It would have been easier to write:

            std::string date1;
            std::getline(std::cin, date1);

Prefer range-based for:

            for (unsigned i = 0; i < length; ++i) {

In modern C++ we prefer the range-based for:

            for (auto const& c: date1) {

Prefer '\n' over std::endl

                    cerr << "Error: Alphabet character found: " << date1[i] << endl;

The difference between the two is a buffer flush to the stream. Manual flushing is discouraged (as it is nearly always wrong). The system will flush the streams for you at the most optimum time.

If you look at most speed question on SO you will find that inappropriate flushing is one of the main reasons for C++ code being slower than C. Remove the inappropriate flushes and the C++ code will get to the same speed as C.

Don't use strtok()

It modifies the input data.

DRY your code.

DRY: Don't repeat your self.

This code is don in two places. It would be better if it was made into a separate method and called from both locations. That way when you want to fix the bug you only have to fix the bug in one location.

            if ((day <= 0) || (day >= 32)) {
                cerr << "Day is limited between 1 and 31, you entered: " << day << endl;
                day = month = year = 0;
                return;
            }
            if ((month <= 0) || (month >= 13)) {
                cerr << "Month is limited between 1 and 12, you entered: " << month << endl;
                day = month = year = 0;
                return;
            }
            if ((year <= 1899) || (year >= 2018)) {
                cerr << "year is limited between 1900 and 2017, you entered: " << year << endl;
                day = month = year = 0;
                return;
        }

Learn to use const methods:

        int get_day(void) {
            return day;
        }
        int get_month(void) {
            return month;
        }
        int get_year(void) {
            return year;
        }

Methods that do not modify the internal state of the object should be marked const; this allows them to be called on const objects. Which then helps when you pass objects by const reference to other methods.

Don't wait to do this later - adding const later has a horrible tendency to cascade changes through the code. Do it now, as you design the class.

Define your own print operator.

    t.print_date();

Why can't I go:

    std::cout << t;

All you need to do is define the output operator:

    std::ostream& operator<<(std::ostream& str, Date const& date)
    {
        date.print_date(str);   // You may want to change `print_date()`
                                // to accept a stream as a parameter so
                                // it can be printed on any stream not
                                // just std::cout
        return str;
    }
\$\endgroup\$
  • \$\begingroup\$ The range tests aren't obviously wrong - but the connection between <= 0 || >= 32 and 1 <= && <= 31 is far from obvious, so the tests could be made to better match the message (or even create a handy check_in_range() function that may throw. \$\endgroup\$ – Toby Speight Dec 12 '17 at 9:27
  • \$\begingroup\$ For writes to std::cerr, I prefer std::endl, to flush the line. For (non-error, non-prompt) output, then \n is usually preferable. Perhaps worth choosing a std::cout write to pick on instead? \$\endgroup\$ – Toby Speight Dec 12 '17 at 9:29
  • \$\begingroup\$ @TobySpeight, I believe std::cerr is not buffered at all? I'll try to find out a reference for this. It seems like it is buffered, so flush is quite appropriate. \$\endgroup\$ – Incomputable Dec 12 '17 at 9:57
0
\$\begingroup\$

Add some more tests

You've provided a main() with some rudimentary, interactive tests. I'd like to see some of the tricky edge cases tested automatically. You'll want to work out how to use the results of these self-tests to return a non-zero value from main() if any of them fail.

Examples of good tests:

  • Last day of 1899 and first day of 2018. (BTW, what's magic about 2018 that should make this class stop working then?)
  • 29th February 1900 and 29th February 2000 (first should be rejected; the second is okay).
  • Calling set_month(February) on 31st May.
  • Calling set_year(1901) on 29th February.
\$\endgroup\$

Your Answer

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