3
\$\begingroup\$

I'm currently doing old exams from my course to train for my upcoming exam. What I'm wondering is about a code block which I will specify after I've presented the different files with code.

//upggift1.cc aka the main

#include "date.h"
#include <iostream>

using namespace std;

int main()
{
    Date d1;
    bool create_date_again = true;
    int temp_date[3];
    char garbage;
    cout << "Enter a date: ";
    cin >> temp_date[0];
    cin >> garbage;
    cin >> temp_date[1];
    cin >> garbage;
    cin >> temp_date[2];

    while (create_date_again)
    {
        try
        {
            Date date{temp_date[0], temp_date[1], temp_date[2]};
            d1 = date;

            create_date_again = false;
        }
        catch (const std::exception &e)
        {
            cout << "invalid date, enter another date: ";
            cin >> temp_date[0];
            cin >> garbage;
            cin >> temp_date[1];
            cin >> garbage;
            cin >> temp_date[2];
            
            create_date_again = true;
        }
    }

    for (int i{}; i < 10000; ++i)
    {
        d1.tomorrow();
    }

    cout << "10000 days later: ";
    d1.print(cout);

    return 0;
}

//date.h

#ifndef DATE_H
#define DATE_H

#include <ostream>

class Date
{
public:
    Date();
    Date(int const y, int const m, int const d);

    bool is_leap_year() const;

    int days_in_month() const;

    void print(std::ostream& os) const;

    void tomorrow();

private:
    int year;
    int month;
    int day;

    void increment();
};
#endif

//date.cc

#include "date.h"
#include <iostream>
#include <string>
#include <array>

using namespace std;

Date::Date()
    : year{}, month{}, day{}
{
}

Date::Date(int const y, int const m, int const d)
    : year{y}, month{m}, day{d}
{
    if (month < 1 || month > 12)
    {
        throw std::domain_error{"Month " + std::to_string(month) + " doesn't exist!"};
    }

    if (day < 1 || day > days_in_month())
    {
        throw std::domain_error{"Day " + std::to_string(day) + " invalid"};
    }
}

int Date::days_in_month() const
{
    static constexpr const std::array<int, 13> days{0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};

    if (month < 1 || month > 12)
        return 0;

    if (month == 2 && is_leap_year())
        return 29;

    return days.at(month);
}

bool Date::is_leap_year() const
{
    if (year % 400 == 0)
        return true;
    if (year % 100 == 0)
        return false;

    return year % 4 == 0;
}

void Date::print(std::ostream &os) const
{
    os << year;
    if (month < 10)
    {
        os << "-0" << month;
    }
    else
    {
        os << "-" << month;
    }
    if (day < 10)
    {
        os << "-0" << day;
    }
    else
    {
        os << "-" << day;
    }
    os << endl;
}

void Date::tomorrow()
{
    day++;
    if (is_leap_year())
    {
        if (day > 29 && month == 2)
        {
            day = 1;
            month++;
            return;
        }
        if (month != 2)
        {
            increment();
        }
    }
    else
    {
        increment();
    }
}
void Date::increment()
{
    if (day > days_in_month())
    {
        day = 1;
        month++;
    }
    if (month > 12)
    {
        month = 1;
        year++;
    }
}

This is the part I'm particularly unsure about (uppgift1.cc file):

while (create_date_again)
    {
        try
        {
            Date date{temp_date[0], temp_date[1], temp_date[2]};
            d1 = date;

            create_date_again = false;
        }
        catch (const std::exception &e)
        {
            cout << "invalid date, enter another date: ";
            cin >> temp_date[0];
            cin >> garbage;
            cin >> temp_date[1];
            cin >> garbage;
            cin >> temp_date[2];
            
            create_date_again = true;
        }
    }

This is how I'm supposed to execute the program, and what is going to happen.

./a.out 
Enter a date:2013-13-02
Invalid date, enter another date:2013-12-42
Invalid date, enter another date:1900-02-29
Invalid date, enter another date:1987-04-13
10000 days later: 2014-08-29
    

My solution to keep making a new date until it's in the right format, is the while loop with try-catch block inside. Is that a solid solution?

I came across the problem that I wanted to create my date inside the try-catch, but then its scope doesn't allow use after the catch. My solution to that was to create d1 in the main function, but I feel like this is kind of wonky? Is there any better way to solve that problem?

Much of the code in the header and implantation files are given and/or should be very similar. Feel free to give my any other advance if you want to and thank you very much if you've spent your time with this!

\$\endgroup\$
7
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Mar 19 at 9:30
  • 2
    \$\begingroup\$ I see you have edited the title, but it still seems to be focused on the mechanism of the program, rather than its purpose. Please re-read How do I ask a good question? and then edit accordingly. \$\endgroup\$ Mar 19 at 14:22
  • \$\begingroup\$ I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ Mar 19 at 15:46
  • \$\begingroup\$ I would use the standard date time functions. Normally you would store the date as seconds from the epoc. Then getting the next day is simply adding 60*60*24 seconds. Adding a thousand days is 1000*60*60*24. Then you only need to convert it into year/month/day when you print it (there are functions for this already). \$\endgroup\$ Mar 19 at 17:39
  • \$\begingroup\$ (@MartinYork: Sure, but how on earth do you add 10000 days?!?!!!!!) \$\endgroup\$
    – greybeard
    Mar 20 at 4:03
4
\$\begingroup\$

upggift1.cc

Please don't do this:

using namespace std;

We have namespaces for good reason, and it's a bad habit to throw away their benefits like that.

Date d1;
bool create_date_again = true;
int temp_date[3];
char garbage;

This looks like something written by a (quite old) C programmer. Prefer to declare variables where they can be initialised, rather than all at the head of the block like this. It looks like temp_date should be three individual variables, since we never actually use them as an array.

cout << "Enter a date: ";

We should be more specific with the message, as users will expect to be able to enter a date in their own locale's usual format, e.g. 19th March 2021, but we require a much more specific format here.

cin >> temp_date[0];
cin >> garbage;
cin >> temp_date[1];
cin >> garbage;
cin >> temp_date[2];

There's no check whether all the inputs were successfully converted. So we don't know whether all four variables were assigned.

    catch (const std::exception &e)

Why are we catching such a broad range of exceptions? In particular, this will catch std::bad_alloc and provide a very poor response to it.

    catch (const std::exception &e)
    {
        cout << "invalid date, enter another date: ";
        cin >> temp_date[0];
        cin >> garbage;
        cin >> temp_date[1];
        cin >> garbage;
        cin >> temp_date[2];
        
        create_date_again = true;
    }

Here, we have a repetition of code we've already written. We shouldn't need to write this input again. And there's still no check for non-numeric input or end of file (giving an infinite loop when I run the program with no input).

And why are we assigning to create_date_again the same value that we already know it contains?


date.h

#include <ostream>

It's better to include <iosfwd> when we only need the declarations.

Date();

Is it a good idea to have a default constructor? That requires a lot of logic in all functions to handle a default-constructed (invalid) date properly. It's better to use std::optional where code needs the concept of "Not a date".

Date(int const y, int const m, int const d);

Declaring the formal parameters const here has no effect, and is therefore just useless clutter.

void print(std::ostream& os) const;

We would normally provide operator<<() instead, for easier use.

void tomorrow();

That's a poor name for a function that modifies the instance.


date.cc

if (month < 1 || month > 12)
{
    throw std::domain_error{"Month " + std::to_string(month) + " doesn't exist!"};

I think std::invalid_argument is a more logical choice there. std::domain_error is more suited to mathematical operations (e.g. matrix inversion).

When we catch the exception in main(), we ought to be extracting and displaying the message to help the user. Otherwise we've just wasted our effort constructing a meaningful message.

bool Date::is_leap_year() const
{
    if (year % 400 == 0)
        return true;
    if (year % 100 == 0)
        return false;

    return year % 4 == 0;
}

That's probably the least efficient way to write this condition. Test the most likely case first:

bool Date::is_leap_year() const
{
    if (year % 4)
        return false;
    if (year % 100)
        return true;
    return year % 400 == 0;
}
if (month < 10)
{
    os << "-0" << month;
}
else
{
    os << "-" << month;
}

Instead of that if/else stuff, just use stream formatting to use 2 digits, zero-filled:

os << '-' << std::setw(2) << std::setfill('0') << month;
os << endl;

Avoid std::endl - just use plain \n and let the caller decide when the stream needs to be flushed.

day++;

Prefer pre-increment when the value isn't used. Although it makes no difference with int, being consistent makes it easy for the reader.

tomorrow() seems over-complicated, particularly since we already have days_in_month() whose logic we seem to be repeating.


Here's something simpler and more robust:

#include "date.h"
#include <iostream>
#include <limits>

static Date get_date(std::istream& is = std::cin, std::ostream& os = std::cout)
{
    for (;;) {
        os << "Enter a date (y-m-d): ";
        char separator;
        int year, month, day;
        is >> year >> separator >> month >> separator >> day;
        if (!is) {
            // discard the erroneous input line
            is.clear();
            is.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            if (is.eof()) {
                throw std::ios_base::failure("Stream read failure");
            }
            os << "Invalid format.";
            continue;
        }
        try {
            return {year, month, day};
        } catch (std::invalid_argument& e) {
            os << e.what() << "  ";
            // and go round again
        }
    }
}

int main()
{
    try {
        auto date = get_date();
        for (auto i = 0;  i < 10000;  ++i) {
            date.advance();
        }
        std::cout << "10000 days later: " << date << '\n';
    } catch (std::runtime_error& e) {
        std::cerr << e.what();
    }
}
#ifndef DATE_H
#define DATE_H

#include <iosfwd>

class Date
{
public:
    Date(int y, int m, int d);

    bool is_leap_year() const;
    int days_in_month() const;

    void advance();

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

private:
    int year;
    int month;
    int day;
};
#endif
#include "date.h"
#include <array>
#include <iomanip>
#include <iostream>
#include <string>

Date::Date(int const y, int const m, int const d)
    : year{y}, month{m}, day{d}
{
    if (month < 1 || month > 12) {
        throw std::invalid_argument{"Month " + std::to_string(month) + " doesn't exist!"};
    }

    if (day < 1 || day > days_in_month()) {
        throw std::invalid_argument{"Day " + std::to_string(day) + " doesn't exist!"};
    }
}

int Date::days_in_month() const
{
    constexpr std::array days{0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};

    if (month == 2 && is_leap_year())
        return 29;

    return days.at(month);
}

bool Date::is_leap_year() const
{
    return year % 4 == 0 && year % 100 || year % 400 == 0;
}

std::ostream& operator<<(std::ostream& os, const Date& date)
{
    return
        os << date.year
           << '-' << std::setw(2) << std::setfill('0') << date.month
           << '-' << std::setw(2) << std::setfill('0') << date.day;
}

void Date::advance()
{
    ++day;
    if (day <= days_in_month()) {
        return;
    }
    ++month;
    day = 1;
    if (month <= 12) {
        return;
    }
    ++year;
    month = 1;
}

With this version, I've separated out a get_date() function. We can test that independently. Normally, I'd use one of the common unit-test frameworks for testing, but we can cobble together something quite simple for this:

#include <sstream>

// return error count - 0 on success
int test_fail(const std::string& input)
{
    std::istringstream is{input};
    std::ostringstream os{};
    try {
        Date d = get_date(is, os);
        // shouldn't get here
        std::cerr << "FAIL: \"" << input
                  << "\" produced " << d
                  << " instead of failure.\n";
        return 1;
    } catch (std::ios_base::failure&) {
        // as expected
        return 0;
    }
}

int test_succeed(const std::string& input)
{
    std::istringstream is{input};
    std::ostringstream os{};
    try {
        get_date(is, os);
        // as expected
        return 0;
    } catch (std::ios_base::failure&) {
        // shouldn't get here
        std::cerr << "FAIL: \"" << input
                  << "\" raised exception.\n";
        return 1;
    }
}

int main()
{
    int errors =
        + test_fail("")
        + test_fail("not a date")
        + test_succeed("not a date\n2000-01-01")
        + test_fail("0-0-0")
        + test_fail("2000-01-0")
        + test_succeed("2000-01-01")
        + test_succeed("2000 01 01")
        + test_succeed("2000+01+01")
        + test_succeed("2000O01O01") // do we really want to allow this??
        + test_succeed("2000-01-31")
        + test_fail("2000-01-32")
        + test_succeed("2000-02-29")
        + test_fail("2000-02-30")
        + test_succeed("2004-02-29")
        + test_fail("2004-02-30")
        + test_succeed("2001-02-28")
        + test_fail("2001-02-29")
        + test_succeed("2100-02-28")
        + test_fail("2100-02-29")
        ;
    if (errors == 1) {
        std::cerr << "There was one test failure.\n";
    } else if (errors) {
        std::cerr << "There were " << errors << " test failures.\n";
    }
    return errors > 0;
}

The testing for the date addition can easily be incorporated into this framework.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback! \$\endgroup\$
    – exer240
    Mar 20 at 8:29
3
\$\begingroup\$

If the goal is to compute a date 10,000 days from now, I would strongly recommend using the std::chrono facilities. If you do, the code would be only a few lines long:

#include <iostream>
#include <iomanip>
#include <ctime>
#include <chrono>

int main()
{
    using namespace std::chrono;
    auto later{system_clock::to_time_t(system_clock::now() + days(10'000))};
    std::cout << "10,000 days from now the date will be "
              << std::put_time(std::localtime(&later), "%F") << "\n";
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the feedback, but as I stated in my description, because many of the files must be in a certain way and atleast exist, this will not be an alternative. One goal of the assignment is train our logic by creating our own function. Your solution is more of a training in memory/googling to find the right #include, I'd guess. \$\endgroup\$
    – exer240
    Mar 20 at 15:49
  • \$\begingroup\$ I'd like to think of it instead as using the most appropriate tool. If I were writing production code, it would look like this (or would use the even more capable std::chrono::year_month_day with a C++20 compliant compiler and library). "Reinventing the wheel" is, however, sometimes a valuable learning experience. \$\endgroup\$
    – Edward
    Mar 20 at 16:02
2
\$\begingroup\$

It's not common practice to use exceptions when validating user input. A simpler approach would be for the Date class to provide a static member function to check whether it's constructible from the given arguments:

class Date
{
public:
    static is_valid(int y, int m, int d);
    Date(int y, int m, int d);

Then instead of throw/catch, we can read input until Date::is_valid() returns true, and then proceed.

If we want better error messages, we could split the function:

    static is_valid_month(int y, int m, int d);
    static is_valid_day(int y, int m, int d);
\$\endgroup\$
1
  • \$\begingroup\$ I will look into that :) \$\endgroup\$
    – exer240
    Mar 20 at 8:30

Your Answer

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

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