patterncppModerate
Enter year, date, and month in any order, I'll organize it
Viewed 0 times
orderyearanyenterorganizedatemonthand
Problem
I would like some advice on how I could make it less repetitive and use more functions and classes.
```
#include
#include
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)
```
#include
#include
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)
Solution
Besides the obvious points already mentioned in the other answers
Don't populate member variables in a constructor from streamed input
Passing
The canonical way is to provide an input operator:
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:
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.
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.
Code Snippets
date(istream &is) { is >> year >> month >> day; }class date {
friend std::istream& operator>>(std::istream& is, date& d) {
is >> d.year >> d.month >> d.day;
return is;
}
// ....
};Context
StackExchange Code Review Q#114468, answer score: 16
Revisions (0)
No revisions yet.