How can I improve this code or make it shorter?

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.

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 &#37; 4 == 0) && (year % 100 != 0) ) || ( year % 400 == 0) ) return ; 
				break ; 
			default:  
				throw "Month is invalid." ; 
			}
		}
	}
	throw "Day is invalid" ; 
}

Todd

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.

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

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

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.

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.

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.

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.

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).

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.