How can I improve this code or make it shorter?

Hi everyone!
This is supposed to take a date in dd/mm/yyyy format from the keyboard and check whether its valid or not.It works for me but just wanted to make it a little shorter(If that's possible) and more readable.

Code:

#include<iostream> using namespace std; int checkyr(int); int checkmon(int); int checkdd(int,int,int); int main() { int dd=1,mm=1,yyyy=1970; cout<<endl<<"Enter the day:"; cin>>dd; cout<<endl<<"Enter Month:"; cin>>mm; cout<<endl<<"Enter year:"; cin>>yyyy; if(!checkyr(yyyy)) { if(!checkmon(mm)) { if(!checkdd(dd,mm,yyyy)) cout<<endl<<"Date is valid"; } } fflush(stdin); cin.get(); return 0; } int checkyr(int yyyy) { if(yyyy>0) return 0; else { cout<<"\nInvalid Year"; return 1; } } int checkmon(int mm) { if((mm>=1)&&(mm<=12)) return 0; else { cout<<"\nInvalid Month"; return 1; } } int checkdd(int dd,int mm,int yyyy) { int lp=0; if((yyyy%4==0 && yyyy%100!=0)||(yyyy%400==0)) lp=1; if((mm%2==1 && mm<=7)||(mm%2==0 && mm>=8)) { if((dd>=1)&&(dd<=31)) return 0; } if(mm==4||mm==6||mm==9||mm==11) { if((dd>=1)&&(dd<=30)) return 0; } if(mm==2) { switch(lp) { case 0: if((dd>=1)&&(dd<=28)) return 0; break; case 1: if((dd>=1)&&(dd<=29)) return 0; break; } } cout<<endl<<"Invalid Day"; return 1; }

Thanks!

Code:

#include<iostream> using namespace std; int checkyr(int); int checkmon(int); int checkdd(int,int,int); int main() { int dd=1,mm=1,yyyy=1970; cout<<endl<<"Enter the day:"; cin>>dd; cout<<endl<<"Enter Month:"; cin>>mm; cout<<endl<<"Enter year:"; cin>>yyyy; if(!checkyr(yyyy)) { if(!checkmon(mm)) { if(!checkdd(dd,mm,yyyy)) cout<<endl<<"Date is valid"; } } fflush(stdin); cin.get(); return 0; } int checkyr(int yyyy) { if(yyyy>0) return 0; else { cout<<"\nInvalid Year"; return 1; } } int checkmon(int mm) { if((mm>=1)&&(mm<=12)) return 0; else { cout<<"\nInvalid Month"; return 1; } } int checkdd(int dd,int mm,int yyyy) { int lp=0; if((yyyy%4==0 && yyyy%100!=0)||(yyyy%400==0)) lp=1; if((mm%2==1 && mm<=7)||(mm%2==0 && mm>=8)) { if((dd>=1)&&(dd<=31)) return 0; } if(mm==4||mm==6||mm==9||mm==11) { if((dd>=1)&&(dd<=30)) return 0; } if(mm==2) { switch(lp) { case 0: if((dd>=1)&&(dd<=28)) return 0; break; case 1: if((dd>=1)&&(dd<=29)) return 0; break; } } cout<<endl<<"Invalid Day"; return 1; }

Try learning how to indent properly.
fflush(stidin) is undefined; read FAQ.
Name your variables appropriately. yyyy, mm, dd is not exactly the best names.

You might try using a lookup table for lengths of months:

Code:

int month_lengths[12] = {31, 28, 31, 30 ... }; if (day >= 1 && day <= month_lengths[month-1])

Except if the month is February, you'll need to check for leap-year and accept 29 days as well.

(This code assumes that month is already validated. You may put an assert in there to be sure that the function is used as it is meant to be.)

In addition it feels strange that you return false if something is valid (and don't use bool instead of int), and the function names could have less ambigous names: is_valid_day, is_valid_month, is_valid_year.

Here's my take.

Code:

#include <iostream> #include <iomanip> using namespace std; // Function Prototypes bool year_ok(int); bool day_ok(int,int,int); int main() { int day = 1, month = 1, year = 1970 ; cout<<endl<<"Enter the day:"; cin >> day ; cout << endl << "Enter Month:" ; cin >> month ; cout << endl << "Enter year:"; cin >> year ; if ( year_ok(year) && day_ok(day, month, year) ) { cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is valid" << endl ; } else { cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is not valid" << endl ; } cin.get(); return 0; } bool year_ok(int yyyy) { if (yyyy >= 0 ) return true ; cout << "Invalid Year" << endl ; return false ; } bool day_ok(int day,int month,int year) { if (day < 1) { cout << "Day is not valid" << endl ; return false ; } switch(month) { case 1: // Jan case 3: // Mar case 5: // May case 7: // Jul case 8: // Aug case 10: // Oct case 12: // Dec if (day <= 31) return true ; break ; case 4: // Apr case 6: // Jun case 9: // Sep case 11: // Nov if (day <= 30 ) return true ; break ; case 2: // Feb if (day <= 28) return true ; if ( day > 29) return false ; // Handle Leap Year if ( ( (year % 4 == 0) && (year % 100 != 0) ) || ( year % 400 == 0) ) return true ; break ; default: cout << "Month is invalid." << endl ; return false ; // Month was not found } cout << "Day is invalid" << endl ; return false ; }

Todd

Here's my take:

Code:

bool year_ok(int year); bool day_ok(int day, int month, int year);

Omitting the names for the variables in the prototypes is just bad practice IMO. It screws up intellisense (no names for the variables!) and it makes it harder to read what functions are available.

Thanks for the feedback.

I think that in this case, it is better to just write a single isValidDate() function that takes the day, month and year. Instead of printing error messages, it could return true if the date is valid, or false if it is invalid. If more precision as to the type of error is needed, it could return an error code of some sort (e.g., an enum). Only in the main() function would error messages be printed.

Thanks!

Quote:

Originally Posted by laserlight

I think that in this case, it is better to just write a single isValidDate() function that takes the day, month and year. Instead of printing error messages, it could return true if the date is valid, or false if it is invalid. If more precision as to the type of error is needed, it could return an error code of some sort (e.g., an enum). Only in the main() function would error messages be printed.

On the other hand, printing errors at the lowest possible level ensures that they ALWAYS get printed even if upper level code forgot to check for errors. Although you need some way of turning them off.

Quote:

Originally Posted by brewbuck

On the other hand, printing errors at the lowest possible level ensures that they ALWAYS get printed even if upper level code forgot to check for errors. Although you need some way of turning them off.

Unless you reuse the function in a program that does not create a console window.

Quote:

On the other hand, printing errors at the lowest possible level ensures that they ALWAYS get printed even if upper level code forgot to check for errors.

It seems to me that exceptions could be used in such a case, though an isValidDate() function that throws an exception if the date is not valid seems rather strange, since I would consider checking an invalid date to be a normal course of events. The very function is designed so that one can check for an error (it serves no other purpose), so to not check for an error code seems... irrational or highly careless.

Quote:

Originally Posted by anon

Unless you reuse the function in a program that does not create a console window.

Like I said, you have to have a way of turning it off.

Quote:

Originally Posted by laserlight

It seems to me that exceptions could be used in such a case,

Putting print statements in the constructors for your exceptions might not be a bad idea. Exceptions are safe in the sense that if they aren't caught, the program terminates abnormally, but I still like to see a specific message printed for every error condition even if the caller is too lazy to do so.

More than once, a customer has found a logic error by observing that the product printed some funny message which didn't correspond to any visible GUI status change. Somebody had forgotten to check an error condition somewhere. Such bugs would never be found (until truly bad things started happening) without those messages.

Even without a console, the messages can go into a log somewhere.

Quote:

The very function is designed so that one can check for an error (it serves no other purpose), so to not check for an error code seems... irrational or highly careless.

In this specific case, yes :)

They will be "turned off" if there is no console window, so you can't say that the error is reported even if you don't check return values. :)

But laserlight has a good point that this function is only called because you are interested in the return value in the first place.

The idea is that you may want to inform the user in different ways in different places of the program, so the function shouldn't do anything more than its immediate task.

Quote:

More than once, a customer has found a logic error by observing that the product printed some funny message

I'm not a professional but wouldn't that be a sign of a very low quality product?

If you failed to check the return value for a function that validates dates, the testers should discover that the program doesn't really validate dates before the product is launched?

Quote:

Originally Posted by anon

The idea is that you may want to inform the user in different ways in different places of the program, so the function shouldn't do anything more than its immediate task.

IMHO this is actually the argument FOR reporting errors at the lowest level. In principle, all error conditions should be reported to the user through the main interface. Printing errors on the console actually HELPS to make sure that's always true. If you see an error message on the console with no corresponding interface change you know that you've missed an error condition somewhere.

Combine this with the concept of a cancellable error message (the upper layer can cancel the printing of the lower layer's message and report the error some other way) and you have a decently robust method of making sure that the application actually checks error conditions.

Even with C++ exceptions a lazy programmer can put empty catch blocks.

Quote:

Originally Posted by anon

I'm not a professional but wouldn't that be a sign of a very low quality product?

I said more than once, but I don't mean "thousands of times" either. It's a QA tool. Most customers don't even run the product in a mode where they could see these messages. The more technically knowledgable ones figure out how to turn them on.

Quote:

If you failed to check the return value for a function that validates dates, the testers should discover that the program doesn't really validate dates before the product is launched?

Assuming the QA testers know what the hell they're doing :(

I like the try/catch/throw approach better.

Code:

#include <iostream> #include <iomanip> using namespace std; // Function Prototypes void year_ok(int year); void day_ok(int day, int month, int year); int main() { int day = 1, month = 1, year = 1970 ; cout<<endl<<"Enter the day:"; cin >> day ; cout << endl << "Enter Month:" ; cin >> month ; cout << endl << "Enter year:"; cin >> year ; try { year_ok(year) ; day_ok(day, month, year) ; cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is valid" << endl ; } catch (...) { cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is not valid" << endl ; } cin.get(); return 0; } void year_ok(int yyyy) { if (yyyy < 0 ) throw "Invalid Year" ; } void day_ok(int day,int month,int year) { if (day > 0) { switch(month) { case 1: // Jan case 3: // Mar case 5: // May case 7: // Jul case 8: // Aug case 10: // Oct case 12: // Dec if (day <= 31) return ; break ; case 4: // Apr case 6: // Jun case 9: // Sep case 11: // Nov if (day <= 30 ) return ; break ; case 2: // Feb if (day <= 28) return ; if ( day == 29) { if ( ( (year % 4 == 0) && (year % 100 != 0) ) || ( year % 400 == 0) ) return ; break ; default: throw "Month is invalid." ; } } } throw "Day is invalid" ; }

Todd

Quote:

I like the try/catch/throw approach better.

I think you are abusing exceptions. Consider:

Code:

void year_ok(int yyyy) { if (yyyy < 0 ) throw "Invalid Year" ; }

If an exception is not thrown, the function has no net effect. In other words, the function either throws an exception or does nothing. In terms of semantics, this means that if all went well, the function is absolutely useless. Only when some error condition unexpected by the caller occurs does it have a net effect.

A quick search of the Web brings up what a blogger has to say about this topic: Exceptions are not exceptional. I do not agree with the terms (i.e., I consider "exceptional" in the sense that it is "unexpected"), but the gist of it is what I have in mind.

Only, you should probably throw an error instead of a string, or maybe a class or struct with the error and a friendly error message to display to the user.

Quote:

Originally Posted by laserlight

I think you are abusing exceptions. Consider:
...
If an exception is not thrown, the function has no net effect. In other words, the function either throws an exception or does nothing. In terms of semantics, this means that if all went well, the function is absolutely useless. Only when some error condition unexpected by the caller occurs does it have a net effect.

It absolutely does something. It tells the rest of the program logic that the year is valid.

Is it an abuse of a feature? Well, that depends on your perspective. I see it as a leverage of a feature. I could of course invent my own messaging system, and add all the supporting logic and communication protocols and structures and global variables, but why, when it is so convenient of a mechanism? The way I see it, throws are there for me, the programmer, to use.

Todd

I just read that blogger post. I hope he feels better now that he's aired that issue he was dealing with, not that is has anything to do with program structure or design. He has a semantic beef.

Todd

Quote:

It absolutely does something. It tells the rest of the program logic that the year is valid.

It does so in an unusual way.

Take a look at:

Code:

try { year_ok(year) ; day_ok(day, month, year) ; cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is valid" << endl ; } catch (...) { cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is not valid" << endl ; }

In the code above, we have two branches of logic. If the year is okay and the day is okay, we print the date with "is valid". Otherwise, we print the date with "is not valid". But the control flow is determined by whether or not year_ok or day_ok throws an exception. I daresay that this is akin to the use of goto. It would be clearer to write:

Code:

if (isValidDate(day, month, year)) { cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is valid" << endl ; } else { cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is not valid" << endl ; }

I think that there is a difference between a function that does two things (despite what the name says) and a function that does one thing and produces some debugging output.

I also think that there is a difference between high level error messages that are meant for the user to see, and low level error messages that should never reach the end user (unless there is some exceptional relationship between software provider and user).

Yes Laserlight, that is much simpler and obviously more straight forward.

My point for the try/catch/throw example was (besides practice for myself) to incorporate the conversions-up-until-then comments about where the detail message should be issued and/or if the detail message should be displayed. With my example, main() can determine if the detail message should be displayed or not.

Todd

Quote:

Yes Laserlight, that is much simpler and obviously more straight forward.

Indeed, because now the program logic is dictated by normal logical structures. As I noted earlier, I think that having the normal program logic dictated by exceptions is an abuse of exceptions.

Quote:

My point for the try/catch/throw example was (besides practice for myself) to incorporate the conversions-up-until-then comments about where the detail message should be issued and/or if the detail message should be displayed.

Ah, and I must say I did start by mentioning exceptions as a possibility myself, heheh. I think anon is right on this one: "there is a difference between high level error messages that are meant for the user to see, and low level error messages that should never reach the end user". As brewbuck has pointed out, the end user might be able to run the program in a mode where he/she can view the low level error messages (or perhaps have them logged).

With the use of exceptions, if the main() function were to print the error message, it would be printing the low level error message contained in the exception caught. This would be fine if there was no other choice, but in the normal course of events, the program should just display an ordinary error message telling the user of the mistake.

Considering the "do one thing and do it well" principle, this means that the function should leave it up to the caller (i.e., main() function) to decide on the error message to show to the user. It might print an error message by itself for logging etc, but that's a low level error message.

Quote:

Originally Posted by laserlight

It does so in an unusual way.

Take a look at:

Code:

try { year_ok(year) ; day_ok(day, month, year) ; cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is valid" << endl ; } catch (...) { cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is not valid" << endl ; }

In the code above, we have two branches of logic. If the year is okay and the day is okay, we print the date with "is valid". Otherwise, we print the date with "is not valid". But the control flow is determined by whether or not year_ok or day_ok throws an exception. I daresay that this is akin to the use of goto. It would be clearer to write:

Code:

if (isValidDate(day, month, year)) { cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is valid" << endl ; } else { cout << setfill('0') << "Date: " << setw(2) << month << "/" << setw(2) << day << "/" << setw(4) << year << " is not valid" << endl ; }

I don't really share your perception that it's as bad as goto.
The use of goto is usually bad because it can create spaghetti code, that is, it can jump just about anywhere. In the try/catch example, we know where the catch block is and can easily see where it jumps if it fails.
That mentioned, I don't see it wrong to use either way. It's a taste thing, so to say.

Quote:

I don't really share your perception that it's as bad as goto.

I do not think that it is as bad as goto either.

Quote:

The use of goto is usually bad because it can create spaghetti code, that is, it can jump just about anywhere. In the try/catch example, we know where the catch block is and can easily see where it jumps if it fails.

The thing is, we are not evaluating exceptions as a language feature on its own. We are comparing it as a means to control the primary logic of a program, instead of being a means to break from the normal flow of a program due to some unexpected error condition (e.g., passing invalid arguments to a function despite the function contract specifying the range of valid arguments).

Quote:

Originally Posted by laserlight

The thing is, we are not evaluating exceptions as a language feature on its own. We are comparing it as a means to control the primary logic of a program, instead of being a means to break from the normal flow of a program due to some unexpected error condition (e.g., passing invalid arguments to a function despite the function contract specifying the range of valid arguments).

I understand, but I also think that throw can be used as either of these. If you use a proper "status" that depicts why the function threw the error, it might just be reasonable. It can also save time (ie if you want to break conditional statements and continue processing later) and avoid goto. I used it once.
Anyway, that's my thoughts on the matter. Try/throw is not only for errors, but can be used to control the logic in a program, as well.