2 replies to this topic

[ash]

Hey guys, I'm just starting out with K&R C and am working through some of the exercises in chapter 1, which were surprisingly challenging for a beginner. On of the exercises, 1-13, is described below: "Exercise 1-13. Write a program to print a histogram of the lengths of words in its input. It is easy to draw the histogram with the bars horizontal; a vertical orientation is more challenging."

So I have just reached the stage of implementing horizontally, and was looking for any feedback on how I implemented it? I see from some of the solutions sites online some of them use a lot more variables than I did, just wondering how my solution could be improved etc. The only material covered up to this point is basic control and arrays, no functions, pointers or anything else yet.

Below is a sample input and what my program generates:

a a a aa a a aa a a ad dg de de deg ad deaeeet et ea e e t e

 

    1: *  *  *  *  *  *  *  *  *  *  

    2: *  *  *  *  *  *  *  *  *  

    3: *  

    4: 

    5: 

    6: 

    7: *  

    8: 

    9: 

>=  10: 

     +  1  2  3  4  5  6  7  8  9 >=10: 

and my code:

#include <stdio.h>


#define NUMVALUES 10							/*max number of specific values to be displayed*/


int charCount = 0;

int wordCount[NUMVALUES];

char input;


int main (int argc, const char * argv[]) {

	

	printf("Enter some text, and then press Enter: \n\n");

	

	/* Get the input and count the length of each word, storing the count for each possibility in array wordCount[]*/

	

	while ((input = getchar()) != '\n') {		/* Loop until user presses enter */

		if (input != ' ') {						/* If the input is not a space, increment the character count by 1 */

			charCount++;

		} else {								/* If the input is a space, a word has just ended. Populate the current count into the array */

			if (charCount < NUMVALUES) {

				wordCount[charCount - 1]++;		/* Increment the content of the array index which matches the count (-1 accounts for array starting at 0)*/

			}

			else if (charCount >= NUMVALUES) {

				wordCount[NUMVALUES - 1]++;

			}

			charCount = 0;						/* Reset the charCount to count the next word's characters */

		}

	}

	

	/* Following sections print out a histogram of * characters, representing the array*/

	

	/* Print y-axis labels for word length, and the *s themselves on correct row*/

	for (int i = 0; i < NUMVALUES; i++) {			/* Print a star for each word of the same length*/

		if (i == NUMVALUES - 1) {

			printf(">=%4d: ", (i + 1));				/* If the number of chars in a word is not the maximum number being accounted for, label row with number of chars in the word */

		} else {

			printf("%5d: ", (i + 1));				/* If the number IS the maximum accounted for, or more, consolidate the count into one row to save space */

		}

		for (int j = 0; j < wordCount[i]; j++) {	/* On each row, print the required number of *s */

			if (j >= NUMVALUES) {

				continue;							/* Limit the number of *s printed to save space in output */

			} else {

				printf("*  ", (j+1));			

			}

		}

		printf("\n");

	}

	

	printf("     +");

	

	/* Print the x-axis labels for the word count */

	for (int i = 0; i < NUMVALUES; i++) {

		if (i == NUMVALUES - 1) {

			printf(" >=%d: ", (i + 1));			

		} else {

			printf("%3d", (i+1));		

		}

	}

	

    return 0;

}

Any feedback/advice/bad habits you guys can see? Going to try and get it working vertically tomorrow.

[dargueta]

A few quick things:

1) Don't use global variables unless you have to. Always be as restrictive as possible with the access you give to variables. For example, if you write a function that takes an array as an argument but doesn't modify the contents, always declare that argument as const. The compiler will then catch you if you try and do something unintentionally. Similarly, you don't want to give two variables the same name and expect a function to modify the correct one if the other is global.

2) charCount starts out as 0. Look at line 21 and see if there might be a problem there.

3) Never assume that anything is initialized to 0. This is not Java. You need initialize each value in wordCount to 0 before you do anything with it.

[kernelcoder]

There is logic problem in your code. Following is the code that draws both horizontal & vertical histogram.

#include <iostream>
#include <vector>
#include <stdio.h>
using namespace std;

int main (int argc, const char * argv[]) 
{
char input;
int wordCount = 0;
int maxLength = 0;

vector<int> words;
words.push_back(0);

cout << "Enter some text, and then press Enter: " << endl << endl;


while ((input = getchar()) != '\n') {           
if (input == ' ') { 
wordCount++;
words.push_back(0); 
} 
else { 
words[wordCount]++;
if (maxLength < words[wordCount]) maxLength = words[wordCount];
}

}

if (words[words.size() - 1] == 0) {
words.pop_back();
}

// Horizontal histogram 
for(int i = 0 ; i < words.size(); i++) {
cout << i + 1 << ": ";
for(int j = 0; j < words[i]; j++) cout <<"* ";
cout << endl;
}


// Vertical histogram
for(int len = maxLength ; len > 0; len--) {
for(int i = 0 ; i < words.size(); i++) {
cout << (words[i] >= len ? "* " : "  ");
}
cout << endl;
}

cout << endl;

for(int i = 0 ; i < words.size(); i++)cout<<"# ";
cout<<endl;

for(int i = 0 ; i < words.size(); i++)cout<<i+1 <<" ";

return 0;
}