Thread: Code Critique
View Single Post
Old Jun 2nd, 2006, 4:59 AM   #2
grumpy
Programming Guru
 
grumpy's Avatar
 
Join Date: Jun 2005
Location: Adelaide, South Australia
Posts: 1,253
Rep Power: 5 grumpy will become famous soon enough
1) Declare your array to be of dimension MAXSIZE rather than MAXSIZE+1, and set loop conditions accordingly. For example, your first loop would be;
   for (i = 0; i < MAXSIZE; ++i)
		arr[i] = 0;

2) Favour preincrement (eg ++arr[nc-1]) over postincrement (eg arr[nc-1]++) unless you need to use the previous value.

3) If the first character in your input is a space, then the logic will be (commented)
   nc = 0;
   while ((c = getchar()) != EOF) {
		++nc;    /*  nc now 1 */
		if (c == ' ' || c == '\n' || c == '\t' || c == EOF){   /*  we've entered a space, so ... */
			--nc; /* nc now zero */
			if (nc <= MAXSIZE) {     /* this test is true */
				arr[nc-1]++;      /*  arr[-1]++ */
so your code exhibits undefined behaviour at this point.

4) Rather than testing against various whitespace characters, try the function isspace() which is in <ctype.h>

5) Compute the max value in your array like this;
        highest = arr[i];
	for (i = 1; i < MAXSIZE; ++i) {    /* end condition corrected as per first tip */
		if (arr[i] > highest)
			highest = arr[i];
	}
This allows the computation to work even if (say) you should change your program so it can hold negative values in the array. Your form will not.

6) Better yet, eliminate the computation of max and simply sort the array (but retain duplicate values). Then the values will contain values in order that you want to print them, and you simply need to count the number of times a value is in sequence to get the value for your histogram. This will be more efficient than what you're doing (printing highest, searching the whole array for that value, decrementing highest, and going over and over again).
grumpy is offline   Reply With Quote