15
\$\begingroup\$

I would like some advice on how I could make it less repetitive and use more functions and classes.

#include <iostream>
#include <string>

using namespace std;

class date
{
public:
    date() = default;
    date(istream &is) { is >> year >> month >> day; }

    bool organizeInfo();
    bool organize();
    ostream &outputInfo(std::ostream &os) { os << year << " " << month << " " << day; return os; }

private:

    bool checkValid();
    bool checkSwitch(unsigned &item1, unsigned &item2, unsigned &item3);
    void switchItems(unsigned &item1, unsigned &item2) { unsigned tempItem1 = item1; item1 = item2; item2 = tempItem1; }

    unsigned year = 0;
    unsigned month = 0;
    unsigned day = 0;
};

inline bool date::checkValid()
{
    if (month <= 12 && day <= 31)
        return 1;
    return 0;
}

inline bool date::checkSwitch(unsigned &item1, unsigned &item2, unsigned &item3)
{
    switchItems(item1, item2);
    if (checkValid())
        return 1;
    else
    {
        switchItems(item1, item2);
        switchItems(item2, item3);
        if (checkValid())
            return 1;
        else
        {
            switchItems(item2, item3);
            switchItems(item1, item3);
            if (checkValid())
                return 1;
            else
            {
                return 0;
            }
        }
    }
}

inline bool date::organize()
{
    if (checkValid())
        return 1;
    else
    {
        if (checkSwitch(day, month, year))
            return 1;
        else if (checkSwitch(year, month, day))
            return 1;
        else if (checkSwitch(day, year, month))
            return 1;
        return 0;
    }
}

bool date::organizeInfo()
{
    if (checkValid())
        return 1;
    else
    {
        if (organize())
            return 1;
        else
            return 0;
    }
    return 0;
}

int main()
{
    cout << "Enter year, month, date and ill try and organize it" << endl;
    date dawg(cin);
    if(dawg.organizeInfo())
        dawg.outputInfo(cout) << endl;
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Why does this have so many upvotes when the basic premise is totally incorrect? It's actually off topic since it's broken code. \$\endgroup\$ – Jim Garrison Dec 20 '15 at 16:47
16
\$\begingroup\$

Besides the obvious points already mentioned in the other answers

Don't populate member variables in a constructor from streamed input

Passing std::istream to populate members in the constructor looks weird:

date(istream &is) { is >> year >> month >> day; }

The canonical way is to provide an input operator:

class date {
     friend std::istream& operator>>(std::istream& is, date& d) {
          is >> d.year >> d.month >> d.day;
          return is; 
     }
     // ....
};

This allows to check the stream state for input errors after using it.

Don't promise things to users of your API, you cannot ever guarantee

As going for the logic you're trying to implement:

You are aware that it's not really possible to distinguish day, month and year inputs just by the bare numbers?

Some test cases in day, month, year order:

  • 1 3 2
  • 5 11 23
  • ...

Well, month must be below or equal 12 and days below or equal 31, but what about days up to 12 or years input up to 12/31?

You actually have to require a specific date format for user input

Users won't be happy with wrong guesses from your code, and what you're promising isn't possible at all.

This is a major flaw in your code, and renders any other discussion merely useless.

\$\endgroup\$
  • 2
    \$\begingroup\$ +1 Don't promise things to users of your API, you cannot ever guarantee any other discussion merely useless \$\endgroup\$ – edc65 Dec 20 '15 at 10:34
11
\$\begingroup\$

I see a number of things that could help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Choose better function names

There are two very similar names organize() and organizeInfo() and it's not obvious without reading the code what the difference might be.

Reconsider the constructor

I found this constructor counterintuitive:

date dawg(cin);

It would have been much easier to understand if it had been written like this:

date dawg;
cin >> dawg;

Use const where practical

Your outputInfo() member fuction does not alter the underlying date, so it should be declared const. In general, whenever you are writing a variable or function prototype look for places you can use const.

Write object I/O functions as inserters and extractors

Instead of a member function outputInfo, write an extractor instead:

friend ostream &operator<<(std::ostream &out, const date& d) { 
    return out << d.year << " " << d.month << " " << d.day; 
}

Similarly, instead of the constructor taking a reference to a std::istream, use a inserter:

friend istream& operator>>(std::istream &in, date& d) {
    return in >> d.year >> d.month >> d.day; 
}

Fix the prompt string

It may seem like a minor issue but the user interface is a very important part of most programs because it's the only part that a user would see. For that reason instead of "and ill try and organize" which soundw like the program is not well, it should be "and I'll try ..."

Use a better algorithm

Right now, if I enter the 10 March 2010 date as 3 10 2010 which is the ordering commonly used in the United States, the program reinterprets that as 3 October 2010. (This was intended to make the same point as @πάντα ῥεῖ in the section "Don't promise things to users of your API, you cannot ever guarantee" but that answer states the point more accurately and eloquently.)

Use better default values

The default constructor creates a date of 0 0 0 which is not really a valid date in most calendrical systems. Better would be to assign a default value which, while possibly not meaningful, would at least be a valid date.

Use true and false for boolean values

Since you're using C++, you should use the keywords true and false instead of 1 and 0 for boolean return values.

Only #include things that are needed

The code currently has this line:

#include <string>

However, there is no code in this program that needs anything from that header, so it should be omitted.

Do more error checking

Dates such as 30 February 2015 could be detected as invalid and rejected, as by throwing an exception.

Use available library functions

Instead of switchItems(), you could use std::swap if you're using C++11 or newer.

Eliminate return 0 at the end of main

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

An alternative approach

As others have correctly noted, some dates are inherently ambiguous and can't always be correctly interpreted. A date of '4 5 1980' might be intended to be 4 May or 5 April. However, any date with day > 12 and year > 31 (which is most dates in the past 2000 years) can be unambiguously rearranged. Further, dates that are invalid, such as 31 April (there are only 30 days in April) can be rejected. Here's an alternative approach that includes a bool ambiguous data member. Leap years are partially handled. The idea is that if the given date is unambiguous, no further action is needed, but an ambiguous date might require, for example, that the user is asked further questions about intent. Here's the refactored code:

#include <iostream>
#include <algorithm>
#include <cassert>

class Date
{
public:
    Date(unsigned y=1970, unsigned m=1, unsigned d=1) :
        year{y}, 
        month{m},
        day{d},
        ambiguous{true}
    { rearrange(); }
    bool operator==(const Date& other) const {
        return year == other.year && month == other.month && day == other.day;
    }
    bool isAmbiguous() const { return ambiguous; }
    friend std::istream& operator>>(std::istream &in, Date& d) {
        in >> d.year >> d.month >> d.day; 
        d.rearrange();
        return in;
    }
    friend std::ostream &operator<<(std::ostream &out, const Date& d) { 
        return out << d.year << " " << d.month << " " << d.day; 
    }

private:
    bool rearrange();
    static bool check(unsigned y, unsigned m, unsigned d);
    static bool invalid(unsigned y, unsigned m, unsigned d);

    unsigned year;
    unsigned month;
    unsigned day;
    bool ambiguous;
};

bool Date::invalid(unsigned y, unsigned m, unsigned d) {
    return y == 0 || m == 0 || d == 0;
}

bool Date::check(unsigned y, unsigned m, unsigned d)
{
    switch (m) {
        case 1:  // January
        case 3:  // March
        case 5:  // May
        case 7:  // July
        case 8:  // August
        case 10: // October
        case 12: // December
            return d <= 31;

        case 4:  // April 
        case 6:  // June 
        case 9:  // September
        case 11: // November
            return d <= 30;

        case 2:  // February
            if (y % 4 != 0) 
                return d <= 28;
            return d <= 29;

        default:
            return false;
    }
}

// returns true if the resulting, possibly rearranged Date is valid
bool Date::rearrange()
{
    // if it's invalid, no rearranging can fix it
    if (invalid(year, month, day)) {
        return false;
    }
    if (day > 31) {
        std::swap(day, year);
    }
    if (month > 31) {
        std::swap(month, year);
    }
    if (month > 12) {
        std::swap(month, day);
    }
    // is it already good?
    if (check(year, month, day)) {
        ambiguous = !((year > 31) && ((month == day) || (day > 12)));
        return true;
    } 
    ambiguous = true;
    return false;
}

void test()
{
    Date anchor{1969, 7, 20};
    std::cout << "anchor : " << anchor << "\n";
    Date ymd{1969, 7, 20};
    assert(ymd == anchor);
    Date ydm{1969, 20, 7};
    assert(ydm == anchor);
    Date myd{7, 1969, 20};
    assert(myd == anchor);
    Date mdy{7, 20, 1969};
    assert(mdy == anchor);
    Date dmy{20, 7, 1969};
    assert(dmy == anchor);
    Date dym{20, 1969, 7};
    assert(dym == anchor);
}

int main()
{
    std::cout << "Enter year, month, Date and I'll try and organize it" << std::endl;
    Date dawg;
    test();
    std::cin >> dawg;
    std::cout << dawg << " is" << (dawg.isAmbiguous() ? "" : " not") << " ambiguous.\n";
}
\$\endgroup\$
  • 4
    \$\begingroup\$ "Use a better algorithm". It parses dates correctly for a majority of the world. I'd call that a good algorithm. \$\endgroup\$ – isanae Dec 19 '15 at 20:08
  • 3
    \$\begingroup\$ While I completely agree that it would be great to have a single format world wide, that's not quite the case today. So "any order" isn't really supported by the program, is it? \$\endgroup\$ – Edward Dec 19 '15 at 20:57
  • 2
    \$\begingroup\$ @isanae There's no common sane order, that's the major flaw of the OPs code. \$\endgroup\$ – πάντα ῥεῖ Dec 19 '15 at 21:14
  • 3
    \$\begingroup\$ @πάνταῥεῖ There is. It's called ISO 8601. \$\endgroup\$ – Ismael Miguel Dec 20 '15 at 8:00
  • 2
    \$\begingroup\$ @AntonGolov The only country with the dates all backwards: USA. \$\endgroup\$ – Ismael Miguel Dec 20 '15 at 15:51
6
\$\begingroup\$
return 1;

Don't return 1 in a boolean method. Return true. The same goes for returning false - don't return 0.


Your indentation is slightly confusing:

else
{
        switchItems(item1, item2);
        switchItems(item2, item3);
        if (checkValid())
        return 1;
    else
    {
        switchItems(item2, item3);
        switchItems(item1, item3);
        if (checkValid())
            return 1;
        else
        {
            return 0;
        }
    }
}

At first glance, I didn't see that the first return 1; was in an if statement because of the indentation issue. You need to un-indent the first three lines here to make your indentation correct.


Use braces, or at least be consistent with your use of braces. If you had used braces, it would have quickly been obvious the return mentioned above was in an if. After the number of bugs resulting in not using braces, you should either use braces or switch to a language that enforces correct indentation for correct behavior (Python is a good one at this).

As for consistency, you sometimes do not use braces on single-line statements, and sometimes do:

if (checkValid())
    return 1;
else
{
    return 0;
}

using namespace std;

Don't bring in the std namespace like this. It is a very good way to have name clashes, which will hopefully throw an error about ambiguous naming, but is not guaranteed too. See this SO post for more information.


I'm not sure why you are inlineing all your functions. First, it is not guaranteed to inline your methods, and the slight potential performance gain is not going to be noticeable in this program. You can read more about this operator at MSDN.


It is a bad idea to instantiate a class, then take input in the constructor. If you need this information for the class, then Dependency Inject (DI) it into the constructor. If you take input in the constructor and there is an error, your class contains bad information and will return garbage data when called.

\$\endgroup\$
  • \$\begingroup\$ Hey thanks for the input I made some changes here they are pastebin.com/wKEbRc7C \$\endgroup\$ – Magirldooger Dec 19 '15 at 19:53
  • \$\begingroup\$ Looks a lot better. However, you can still fix some other things, such as changing the if (condition) { return true; } else { return false; } statements to return condition;. \$\endgroup\$ – Hosch250 Dec 19 '15 at 19:55
  • \$\begingroup\$ I dont understand return condition; \$\endgroup\$ – Magirldooger Dec 19 '15 at 20:00
  • \$\begingroup\$ Caridorc's answer mentions this. An example is changing if organize()) { return true; } else { return false; } to return organize();. \$\endgroup\$ – Hosch250 Dec 19 '15 at 20:02
6
\$\begingroup\$

Simplify boolean returns

if (month <= 12 && day <= 31)
    return 1;
return 0;

Is equivalent to:

return month <= 12 && day <= 31;

But the latter is shorter and cleaner.

The same goes for:

        if (checkValid())
            return 1;
        else
        {
            return 0;
        }

-> return checkValid()

A more substantial simplification goes from:

if (checkValid())
    return 1;
else
{
    if (checkSwitch(day, month, year))
        return 1;
    else if (checkSwitch(year, month, day))
        return 1;
    else if (checkSwitch(day, year, month))
        return 1;
    return 0;
}
}

To:

return checkValid() || 
       checkSwitch(day, month, year) ||
       checkSwitch(year, month, day) ||
       checkSwitch(day, year, month);

And even another one:

bool date::organizeInfo()
{
    if (checkValid())
        return 1;
    else
    {
        if (organize())
            return 1;
        else
            return 0;
    }
    return 0;
}

to:

return checkValid() || organize();
\$\endgroup\$

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.