Programming Forums > Application Development > C++

C.F.Gauss · Feb 13th, 2006, 9:48 PM

I would appreciate it if you (who ever that is) would take the time to look over my homework assignment. It's still a work in progress as it's not due for another week or so. But I want this program to be PERFECT so I'm looking for ways to improve it, and catch any errors or bad style.

The purpose of the assignment is to get practice using pointers and to dynamically allocate as much memory as the user needs. I need to recover the memory using the delete operator and make sure there were no memory errors.

I still need to implement input validation, but I'll do that a bit later. I also need some more comments so at this point I'm mostly looking to make sure everything is working as it should be.

Thanks for your time.

The Dark · Feb 14th, 2006, 12:03 AM

Looks OK to me after a quick look. There a couple of things I didn't like:
1. in the get_grades function, you use

(Toggle Plain Text)

cin >> *(grades + i);

in other functions you use

(Toggle Plain Text)

grades[i]

which I find easier to follow. They both work, but it just looks odd to have both styles in there.

2. I don't think average_score should be printing out the sorted list (unless that is defined in your homework). I think it would be better to have a different function that prints out the list, and call that. The new function shouldn't print the "The sorted list of grades entered is:" message, that should be in the top level. That way you can reuse the function for printing unsorted lists later on.

Both of these are only minor nitpicks, and probably just my personal preference.

C.F.Gauss · Feb 15th, 2006, 4:16 PM

Quote:

Originally Posted by The Dark

...snip...

Thanks for the suggestions! I took those into account...

I've made some changes and included user input validation. I'm pretty sure everything should work.

Of course I still need more comments, but I'm mostly there.

The Dark · Feb 15th, 2006, 5:46 PM

I am not sure what the CIsDigit is trying to achieve - is it just checking for a valid integer? If so, you don't need to write the string to a file and read it back in, have a look at the stringstream class, which will let you do that directly from a string. This is still a very complex way to do it though, search the forums for input checking, there are a number of threads on it.

If you do decide you need a class to do the checking, you should probably create it when you need it (unless this is too inefficient). Also this function declaration:

(Toggle Plain Text)

void get_grades(int *grades, int scores, CIsDigit &Grades);

It is a bad idea to have two parameters to your function with the same name, except for case. Someone looking at your program (e.g. me) would have a hard time telling what the second Grades parameter is for. If you need to pass it in, call it inputValidator, or even isDigit.

Also, if you have a class that creates a temp file (such as your CIsDigit currently does), it is good practise to delete the file in the destructor, rather than relying on the calling program to know to call DeleteTempFile(). You can still have the DeleteTempFile method available, but you shouldn't require the user of your class to know about its internal workings.

I would also change the name of display_sorted_data to display_data, as it doesn't matter to the display function whether the data is sorted or not.

Animatronic · Feb 15th, 2006, 6:24 PM

I agree with The Dark on validating your input, its alot easier checking the state of whatever stream your getting your data from.

(Toggle Plain Text)

int * p = new int[1000];
if(p==NULL)
{
//error
}

This isn't going to work, new throws a std::bad_alloc exception on an error br default it dosen't return 0. e.g.

(Toggle Plain Text)

	try
	{
		int * p = new int [100000000];
	}
	catch( std::bad_alloc & e )
	{
		//...
	}

You cant get it to return 0 on error by passing std::nothrow to new:

(Toggle Plain Text)

	int * p = new(std::nothrow) int [100000000];
	if(!p)
	{
		//...
	}

Eric the Red · Feb 15th, 2006, 8:00 PM

is this high school/ college or university work?

Feb 14th, 2006, 12:03 AM	#2
The Dark Expert Programmer Join Date: Jun 2005 Posts: 852 Rep Power: 4	Looks OK to me after a quick look. There a couple of things I didn't like: 1. in the get_grades function, you use (Toggle Plain Text) cin >> (grades + i); cin >> (grades + i); in other functions you use (Toggle Plain Text) grades[i] grades[i] which I find easier to follow. They both work, but it just looks odd to have both styles in there. 2. I don't think average_score should be printing out the sorted list (unless that is defined in your homework). I think it would be better to have a different function that prints out the list, and call that. The new function shouldn't print the "The sorted list of grades entered is:" message, that should be in the top level. That way you can reuse the function for printing unsorted lists later on. Both of these are only minor nitpicks, and probably just my personal preference.

Feb 15th, 2006, 5:46 PM	#4
The Dark Expert Programmer Join Date: Jun 2005 Posts: 852 Rep Power: 4	I am not sure what the CIsDigit is trying to achieve - is it just checking for a valid integer? If so, you don't need to write the string to a file and read it back in, have a look at the stringstream class, which will let you do that directly from a string. This is still a very complex way to do it though, search the forums for input checking, there are a number of threads on it. If you do decide you need a class to do the checking, you should probably create it when you need it (unless this is too inefficient). Also this function declaration: (Toggle Plain Text) void get_grades(int grades, int scores, CIsDigit &Grades); void get_grades(int grades, int scores, CIsDigit &Grades); It is a bad idea to have two parameters to your function with the same name, except for case. Someone looking at your program (e.g. me) would have a hard time telling what the second Grades parameter is for. If you need to pass it in, call it inputValidator, or even isDigit. Also, if you have a class that creates a temp file (such as your CIsDigit currently does), it is good practise to delete the file in the destructor, rather than relying on the calling program to know to call DeleteTempFile(). You can still have the DeleteTempFile method available, but you shouldn't require the user of your class to know about its internal workings. I would also change the name of display_sorted_data to display_data, as it doesn't matter to the display function whether the data is sorted or not.

Feb 15th, 2006, 6:24 PM	#5
Animatronic Programmer Join Date: Jun 2005 Posts: 99 Rep Power: 4	I agree with The Dark on validating your input, its alot easier checking the state of whatever stream your getting your data from. (Toggle Plain Text) int * p = new int[1000]; if(p==NULL) { //error } int * p = new int[1000]; if(p==NULL) { //error } This isn't going to work, new throws a std::bad_alloc exception on an error br default it dosen't return 0. e.g. (Toggle Plain Text) try { int * p = new int [100000000]; } catch( std::bad_alloc & e ) { //... } try { int * p = new int [100000000]; } catch( std::bad_alloc & e ) { //... } You cant get it to return 0 on error by passing std::nothrow to new: (Toggle Plain Text) int * p = new(std::nothrow) int [100000000]; if(!p) { //... } int * p = new(std::nothrow) int [100000000]; if(!p) { //... }

Thread Tools
Show Printable Version Email this Page
Display Modes
Linear Mode Switch to Hybrid Mode Switch to Threaded Mode

Feb 15th, 2006, 8:00 PM	#6
Eric the Red Hobbyist Programmer Join Date: Feb 2006 Posts: 214 Rep Power: 0	is this high school/ college or university work?

Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)

Programming Forums > Application Development > C++

Look over my progie