I've just finished my homework program and I'm looking for any critiques. Whether they be about my methods, format of how the code is spaced and such, or anything of the sort. Also some ideas on decent commenting would be helpful.

In summary I'm trying to learn some good programming habits.

The assignment was to fill a 1 dimensional array with 5-10 numbers, compute the average, store the absolute deviation from the original numbers in another array. Then to print the Original array, average, deviated array, the largest deviation and least deviation. The largest and least had to also print their original value and the position in the array they occupy.

// CI_230_Assignment2.cpp : Defines the entry point for the console application.

//Assignment 2

#include "stdafx.h"
#include <iostream>
using namespace std;
#include <cmath>


//========================================================

void userInput( int size, float *input ){
	cout << "Please enter " << size << " values: \n";
	for( int i = 0; i < size; i++ ){
		cin >> input[i];
	}
}//end userInput

//========================================================

void find_average( int size, float &average, float *input ){//compute and return average according to const int SIZE, the determining var for size of array
	float sum = 0.0;
	for( int i = 0; i < size; i++ ){
		sum = sum + input[i];
	}
	
	average = sum / size;
		
}//end_find_average

//========================================================

void fillDev( int size, float *average, float *input, float *deviation ){
	for ( int i = 0; i < size; i++ ){
		deviation[i] = abs( input[i] - *average );
	}
}//end_fillDev

//========================================================

void find_deviation( int size, float *leastDev, float *largeDev, float *deviation, int *positionLeast, int *positionLarge ){
	for ( int i = 0; i < size; i++ ){//finds the least and largest deviation and stores them in their respective variables
		if ( deviation[i] > *largeDev ){
			*largeDev = deviation[i];
			*positionLarge = i;
		}
		else if ( deviation[i] < *leastDev ){
			*leastDev = deviation[i];
			*positionLeast = i;
		}
	}
}//end find_deviation
		

//========================================================

void output( int size, float *leastDev, float *largeDev, float *average,
			float *input, float *deviation, int *positionLeast, int *positionLarge ){

	cout << "\nOriginal Array: ";
	for ( int i = 0; i < size; i++ ){//prints original array
		cout << input[i] << " ";
	}

	cout << "\n\n" << "Average: " << *average << endl;
	
	cout << "\nDeviation Array: ";
	for ( int i = 0; i < size; i++ ){//Prints deviation array
		cout << deviation[i] << " ";
	}
	cout << "\n\n" << "Largest Deviation: " << *largeDev << " belonging to " 
		 << input[*positionLarge] << " in position " << *positionLarge << " of Original Array." << "\n";

	cout << "\n" << "Least Deviation: " << *leastDev << " belonging to " 
		 << input[*positionLeast] << " in position " << *positionLeast << " of Original Array." << "\n";

	system("PAUSE > nul");
}//end_output

//========================================================

void main(){
	const int SIZE = 10;
	
	int positionLeast, positionLarge; //Holds which location of the array is the least deviation and greatest deviation respectivly
	float average, leastDev = 999.0, largeDev = -999.0;
	float input[SIZE];
	float deviation[SIZE];
	
	userInput( SIZE, input );
	find_average( SIZE, average, input );
	fillDev( SIZE, &average, input, deviation );
	find_deviation( SIZE, &leastDev, &largeDev, deviation, &positionLeast, &positionLarge );
	output( SIZE, &leastDev, &largeDev, &average, input, deviation, &positionLeast, &positionLarge );
	
}//end_main
VernonDozier commented: Code tags on first post! +24

Recommended Answers

All 7 Replies

  1. Thanks for using code tags on your first post.
  2. Thanks for formatting your code on your first post.
  3. You used meaningful variable names. That's good. People can follow your code more easily that way.
  4. You had a paragraph explaining what you did here. Perhaps put that at the top of the code itself too as comments? And maybe have something similar display when your run it. I, the user, am told to enter 10 numbers, but I have no idea why. i probably know already, but in case I don't, it can't hurt to explicitly display that in the program. Not the "how" so much (i.e. store it in an array, copy the array, etc.) since the user doesn't need or want to know the "how", but the "what" - say that the program calculates the average and the deviations and the largest/smallest deviation).
  5. void main () - should be int main () - see link below.
  6. system ("PAUSE") - avoid it. See link below.
  7. The spec says I can enter 5 - 10 numbers, but the program requires me to enter 10. I am not allowed to enter only 5.
  8. stdafx.h - Consider removing this. Don't assume everyone is using Visual Studio. Some people will have to remove this to make it compile.

http://www.gidnetwork.com/b-66.html
http://www.gidnetwork.com/b-61.html

Thank you very much for the helpful advice. At the moment, I'm not sure how to make an array that you change the value of. I took a look at the links, I previously had been using cin.get() for the system("pause"), I saw it once or twice in some code and it stuck with me. I'll start looking into dynamic arrays. As for the void main(), I guess I was misinformed.

Oh, and I noticed sometimes that when accepting a variable without the use of cin.ignore() the cin.get() wouldn't work. I'm not sure why this, I suppose I should look it up. All I knew is that the exe would close without it.

Thank you very much for the helpful advice. At the moment, I'm not sure how to make an array that you change the value of. I took a look at the links, I previously had been using cin.get() for the system("pause"), I saw it once or twice in some code and it stuck with me. I'll start looking into dynamic arrays. As for the void main(), I guess I was misinformed.

Oh, and I noticed sometimes that when accepting a variable without the use of cin.ignore() the cin.get() wouldn't work. I'm not sure why this, I suppose I should look it up. All I knew is that the exe would close without it.

Regarding the cin.get (), >> leaves a newline in the stream and get takes it, so it won't pause. That's why the ignore is needed. See these threads for why.

http://www.dreamincode.net/forums/index.php?showtopic=30581
http://www.daniweb.com/forums/thread90228.html

Regarding the dynamic array, they're useful and you'll need to learn them, so go for it, but they aren't necessary for your program. Your max size is 10, so make your array size 10. But have another variable with the actual size and pass that to your functions.

const int MAX_SIZE = 10;
	float input[MAX_SIZE];
	float deviation[MAX_SIZE];

        int SIZE;
        cout << "How many numbers do you want to enter : ";
        cin >> SIZE;

Everything else stays the same.

I changed some stuff around, I changed userInput() and main(). Also, thanks for the snippet, I had no idea you could do that. Just one of those things that wouldn't have come to mind, but then again that could probably be chalked up to experience.

Oh, and I was also curious if there is a better way to make leastDev and largeDev accept values, besides increasing/decreasing (respectively) the initialization value. I suppose I could always just limit the range of accepted values.

Thanks again for your time and advice.

//========================================================

void userInput( int *size, float *input ){
	cout << "How many numbers do you want to enter (Between 5 and 10) : ";

	while ( *size > 10 || *size < 5 ){//check for valid user input
		cin >> *size;
		if ( *size > 10 || *size < 5 ){//display error message if read value is out of range
		cout << "\aSorry, an integer value between 5 and 10 must be used.  ENTER to continue...";
		cin.ignore();
		cin.get();
		cout << "How many numbers do you want to enter (Between 5 and 10) : \n";
		}
	}
	cout << "\nThey will be averaged and then printed by their deviation to the mean.\n";
	cout << "The Least and Largest deviation will be printed.\n";
	cout << "\nPlease enter " << *size << " values\n";
	for( int i = 0; i < *size; i++ ){//allows the filling of the original array
		cin >> input[i];
	}
}//end userInput

//========================================================

int main(){
	//system("color 3A");
	int positionLeast, positionLarge; //Holds which location of the array is the least deviation and greatest deviation respectivly
	float average, leastDev = 99999.0, largeDev = -99999.0;
	
	const int MAX_SIZE = 10;
	float input[MAX_SIZE];
	float deviation[MAX_SIZE];

    int SIZE;
    	
	userInput( &SIZE, input );
	find_average( SIZE, average, input );
	fillDev( SIZE, &average, input, deviation );
	find_deviation( SIZE, &leastDev, &largeDev, deviation, &positionLeast, &positionLarge );
	output( SIZE, &leastDev, &largeDev, &average, input, deviation, &positionLeast, &positionLarge );
	
	cin.ignore();
	cin.get();
	return 0;
	
}//end_main

Oh, and I was also curious if there is a better way to make leastDev and largeDev accept values, besides increasing/decreasing (respectively) the initialization value. I suppose I could always just limit the range of accepted values.

Don't limit the range. You've written it with the assumption that leastDev and largeDev will be immediately overwritten to equal the first element's value, so just initialize them to be the first element's value and then compare them to everything else. That way there's no concern about what to do if you get real deviations of 999999 or something and get a bogus answer. This way you're guaranteed a REAL answer.

void find_deviation( int size, float *leastDev, float *largeDev, float *deviation, int *positionLeast, int *positionLarge )
{
			*largeDev = deviation[0];
			*positionLarge = 0;
			*leastDev = deviation[0];
			*positionLeast = 0;

	for ( int i = 1; i < size; i++ ){//finds the least and largest deviation and stores them in their respective variables
		if ( deviation[i] > *largeDev ){
			*largeDev = deviation[i];
			*positionLarge = i;
		}
		else if ( deviation[i] < *leastDev ){
			*leastDev = deviation[i];
			*positionLeast = i;
		}
	}
}//end find_deviation

Note that i starts at 1 now, not 0. There's no need to compare the 0th element to itself.

Oh, I'm kicking myself for that one now. Thanks again, and on a semi-related note, I'm new to the forum as is made obvious by my label. How long would be a decent time to leave this open? Being a "better my knowledge" kind of thread versus a distinct problem, I don't know as though it qualifies for the Mark as solved selection.

I apologize for being off topic, and yet again thanks for the assistance.

Oh, I'm kicking myself for that one now. Thanks again, and on a semi-related note, I'm new to the forum as is made obvious by my label. How long would be a decent time to leave this open? Being a "better my knowledge" kind of thread versus a distinct problem, I don't know as though it qualifies for the Mark as solved selection.

I apologize for being off topic, and yet again thanks for the assistance.

No apology necessary. It's completely up to you. As you mentioned, it's a general knowledge kind of thread. Those are much more open ended and free flowing as opposed to "I got an error on line 18", then someone explains why and the problem's solved. For problems like that, generally mark them solved as soon as they're solved. This thread's not really like that, so you can leave it open if you like and see if anyone else has any input. I'd leave it open for now and if no one has commented for 24 hours or so, it'll fall off the front page and then no one will see it again, so mark it solved then.

Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.