Suggests and solution to Histogram program problem please

[Robin_Dragon]

Hey peeps, i am just another lost programmer here looking for some good old fashion help, pertaining to a histogram program.
If there is anyone out there that can follow me as to what i will me explaining i would like it if u'd step in make
a suggestion or plainly help me solve this problem. Are u ready kids....GRRRRRRRREAT!!!!
I want to be able to use the getData algorithm to read the file and store the data in an array, (still with me....GOOD).
Then be able to use the printData algorithm to print the data in the array(asleep yet...GREAT). Thirdly, using the
makeFrequency algorithm to examine the data in the array, one element at a time, then add 1 to the corresponding element ina
frequency array based on the data value (ya'll still there right?...FABULOUS).
Finally,use makeHistogram algorithm to print out a vertical histogram using asterisks for each occurrence of an element.
Uh like...for eg. if there were like five value 1s and eight value 2s in the data, it would print out:

1: *****

2: ********

P.S. This is an example of the program but i am somehow going at it wrong because of the books i am using. So if any one can
help me please, and tell me what i need to do to better it, i would really appreciate it. Thanx a million.:-)

#include <iostream>
#include <ctime>
using namespace std;

void display( int[], int );

int main()
{
	const int arraysize = 10;
	srand( time( 0 ) );
	int val, array[ arraysize ] = { 0 }; // initialize the ten element "array" to zero
	
	for( int i = 0; i <= 500; i++)
	
	{
		val = 0 + rand() % 99;
		array[ val / 10 ] = val;
	}
	
	display( array, arraysize );


}//end main

void display( int a[], int arraysize )
{
	int minimum = 0,maximum =  9; // Declare and initialize maximum and minimum of the first range

	for( int i = 0; i < arraysize; i++ )
	{
		cout << minimum << "-" << maximum << "  " << a[ i ]; //output range
		
		for( int j = 0; j < a[ i ]; j++ )
			cout << "*"; //output frequency
		
		cout << endl << endl;
		
		minimum = minimum + 10; // Increment minimum by 10
		maximum = maximum + 10;	// Increment maximuim by 10
	}
	system("PAUSE");
	return;
}

[Narue]

>tell me what i need to do to better it
Be careful what you wish for.

Your actual problem is that you aren't making a histogram out of a frequency table. A frequency table is the count of occurrences, which is not what you're doing here:

array[ val / 10 ] = val;

What you want to do is increment the appropriate index rather than assign the random value to it:

++array[ val / 10 ];

Now that it's working, I'll tell you how to make your code better. :twisted:

>void display( int[], int );
It's a good idea to name your parameters. The definitions aren't always available, so you'll need to figure out how to use a function solely from the declaration.

>srand( time( 0 ) );
While you might not care at the moment, this isn't a very solid way to seed the random number generator. See this for more details than you could ever want.

>int val, array[ arraysize ] = { 0 };
It's generally a bad idea to declare multiple variables on the same line, especially when one or more of those variables are complex. The exception is when the variables are simple, it's obvious there's more than one, and they are very closely related.

>// initialize the ten element "array" to zero
Quotations in this context typically mean that you're saying array, but it's not really an array, you're just pretending it's an array. Which is quite confusing because the ten element "array" actually is an array. Also, since you're using a comment for one variable but not another, they need to be split into separate lines.

>for( int i = 0; i <= 500; i++)
If you really wanted 501 numbers, that's okay. But keep in mind that it's customary in C++ to use a half-open range where the end value is exclusive and not a value produced by the range. [0,10) means that the range is {0,1,2,3,4,5,6,7,8,9}. You do this with a loop like so:

for ( int i = 0; i < 10; i++ )

A closed range has the end value inclusive. [1,10] means that the range is {1,2,3,4,5,6,7,8,9,10} and can be loopified like so:

for ( int i = 1; i <= 10; i++ )

Both of those ranges have ten items, and if you mix them up, you may accidentally create an off by one bug.

>val = 0 + rand() % 99;
I've already linked to the Using rand() article, so be sure to read it because this is a potential issue as well. Also, adding 0 doesn't do anything in this expression.

>}//end main
Ugh. I can't begin to tell you how much I despise block end comments. They're pointless when the blocks are well written and when the blocks aren't well written, they're like tossing diamonds on a pile of crap. Even though the diamonds help a little, it's still a steamy pile of crap.

Do yourself a favor. Keep your blocks clean and well modularized, and get rid of the block end comments.

>int minimum = 0,maximum = 9;
This is an example of closely related variables that can be declared on the same line.

>// Declare and initialize maximum and minimum of the first range
But that comment really has no business telling me what the code just did.

>//output range
This doesn't tell me anything useful.

>//output frequency
This doesn't tell me anything useful.

>cout << endl << endl;
Beware the endl, it does more than you think. endl will print '\n', that's sure, but it also flushes the stream every single time you use it. You don't need to flush often, so I recommend getting into the habit of printing '\n' directly instead of using endl.

>minimum = minimum + 10; // Increment minimum by 10
>maximum = maximum + 10; // Increment maximuim by 10
Both of those comments are pointless. They just repeat what the code tells me. Also, you can improve those operations by using something more conventional:

minimum += 10;
maximum += 10;

It saves keystrokes, thus delaying your inevitable carpal tunnel by a few more years.

>system("PAUSE");
Don't ever do this again. It's slow, it's insecure, and it's non-portable. If your IDE isn't smart enough to keep the window open until you've read the ouput, either run the program from the command line, or use this little function:

#include <iostream>
#include <ios>
#include <limits>

void jsw_pause( bool dirty_stream )
{
  std::cout<<"Press [Enter] to continue . . .";

  if ( dirty_stream ) {
    // Discard leftover characters in the stream
    std::cin.ignore ( std::numeric_limits<std::streamsize>::max(), '\n' );
  }

  std::cin.get();
}

Pass false only if you're sure that there aren't any leftover characters in the stream. That's there because cin.ignore will block once if the stream is empty, and cin.get will block again afterward. If the stream is empty, you'll have to hit Enter twice.

[Robin_Dragon]

thanx man, i gonna go try it out. And tell u how it went.