# Trying to count words in a file, keep getting double

This topic is 4793 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Ok. I'm making hangman. The program is supposed to choose a word at random from a file called "words.txt". The first step is to find out how many words are in the file, then resize an array to that size, fill it up with words from the file and then shuffle the order of the array (so the player won't get repeats during the same game). I don't know enough to know if that is an unduly convoluted set up (if it is go ahead and be honest). Either way though, the method I use to count the number of words always delivers twice the actual number of words. I could simply divide by two but looking at my code it is not apparent to me why it keeps counting twice the number of words and I'd really rather not have to compensate for bad code. So here it is, you have to make a .txt file called words.txt in the same directory. Obviously with some words in it.
#include <iostream>
#include <cstdlib>
#include <fstream>
using namespace std;

int getCount();
int countWords = 0;

int main()
{
getCount();
if(getCount()==1)
{
cout << "\n\tError: The file words.txt cannot be found.\n"
<< "\tPlease make sure that there is a file by that\n"
<< "\tname in the same directory as this program.\n\n\t";
system("pause");
return 1;
}
cout << countWords << endl;
system("pause");
}

int getCount()
{
string word;

{
return 1;
}

{
countWords++;
}
return 0;
}



##### Share on other sites
Why are you calling the function twice?

Quote:
 then resize an array to that size

Why not use a vector or a deque?

#include <iterator>#include <iostream>#include <fstream>#include <vector>int main(){   std::ifstream ifs("words.txt");   std::istream_iterator<std::string> ifs_begin(ifs);   std::istream_iterator<std::string> ifs_end;   std::vector<std::string> words(ifs_begin, ifs_end);   std::cout << words.size() << std::endl;}

##### Share on other sites
Quote:
 Original post by FrunyWhy are you calling the function twice?

Hehe, yep, that's the answer right there.

You see this little code right here?
getCount();

That calls getCount() once. You knew that.

However, see this bit below it?
if(getCount()==1)

This calls getCount() as well. Delete the first call and you're good to go.

-Greven

##### Share on other sites
What Fruny is trying to point out are these lines:
     getCount();     if(getCount()==1)

You call getCount, the call it again, to see if it equals 1. What you want to do in the if statement is check againts countWords, not getCount(). That is why you have double -- you are calling it twice.

[Edited by - visage on January 7, 2005 10:13:32 PM]

##### Share on other sites
I didn't notice that I was...

##### Share on other sites
Funny - most people catch that kind of issue themselves ... Right after they hit "post".

I know it does for me.

Wholly

##### Share on other sites
Quote:
 Original post by visageWhat Fruny is trying to point out are these lines: getCount(); if(getCount()==1)You call getCount, the call it again, to see if it equals 1. What you want to do in the if statement is check againts countWords, not getCount(). That is why you have double -- you are calling it twice.

Nah, he just needs to delete the first getCount() call. More elegant that way. Plus, the way you suggest wouldn't work anyway (well, OK, it would if you changed the "==1" to "==0" as well...). See the getCount() function? It retuns 0 on success and 1 on fail, which is what he is testing for.

##### Share on other sites
D'oh!

Why does the if statement call it again? I thought it was asking if the value was 1.

Quote:
 Why not use a vector or a deque?

I didn't know how to, until about 30 secs ago. :D

##### Share on other sites
Quote:
 Original post by skulldrudgeryWhy does the if statement call it again? I thought it was asking if the value was 1.

It calls the function and checks ask if the return value is 1.

Quote:
 I didn't know how to, until about 30 secs ago. :D

Time to learn. [wink]

Incidentally, problems like this one is one of the reason why people recommend avoiding global variables: it was not obvious from the function's interface that it was stateful and would keep adding to the previously tallied word count.

##### Share on other sites
Ok, crisis averted. It works now. This also explains an earlier problem I had where "Press any key to continue" from system("pause") also came up twice. I used to have it at the end of the getCount() function. Thanks for the replies!

##### Share on other sites
Quote:
 Original post by skulldrudgeryOk, crisis averted. It works now. This also explains an earlier problem I had where "Press any key to continue" from system("pause") also came up twice. I used to have it at the end of the getCount() function. Thanks for the replies!

I really don't like system("pause");, if only because it prevents your programs from being used as part of shell scripts/batch files/pipes. If I want the display to pause, I'll put a command to that effect outside of the program (in a shell script/batch file).

##### Share on other sites
Quote:
 Original post by FrunyIncidentally, problems like this one is one of the reason why people recommend avoiding global variables: it was not obvious from the function's interface that it was stateful and would keep adding to the previously tallied word count.

I'm just not there yet. I'm on my way.

##### Share on other sites
Quote:
 Original post by skulldrudgeryI'm just not there yet. I'm on my way.

And I'm waiting with a whip to set you back on track if you deviate from the path. [evil]

##### Share on other sites
If you use ignore, you can set it up for a batch file without using pause. It will still wait for enter though. For instance
#include <iostream>int main() {    std::cin.ignore(1000, '\n');	return 0;}

Then, on the command line, you can put
echo | whatever.exe
This will pass a newline character to your program as it is run, for use in batch scripts. You can pass other info at the same time, if you want.

[Edited by - Eric Burnett on January 8, 2005 12:08:42 AM]

That's horrible.

##### Share on other sites
Getting rid of the global variable is easy; just have getWords() return the number of words. This is how functions are supposed to behave; they report the result of their calculation. (Hence the name 'function' - same idea behind the name here as in mathematics.) Void return values should be infrequent in practice. (Things are different for object methods, which will often be more about changing the object's state than about reporting a calculation result.)

Also, getCount() shouldn't be responsible for the actual opening of the file stream. (That then frees you up to use the return value for its natural purpose.)

int main() {  ifstream loadWords("words.txt");  if(!loadWords) {    cout << "\n\tError: The file words.txt cannot be found.\n"         << "\tPlease make sure that there is a file by that\n"         << "\tname in the same directory as this program.\n\n\t";    return 1;  }  cout << getCount(loadWords) << endl;  loadWords.close();}int getCount(ifstream input) {      string word;  int count = 0; // now the count of words is local, and will restart at 0  // each time the method is called. So now we can count multiple streams.  while(!input.eof()) {    input >> word;    count++;            }  return count;}

See how much cleaner? :)

Of course, this is just to drive home the point about code organization and how to avoid globals where they aren't needed. To solve your real problem, you should go with the strategy Fruny outlined - std::vector is a very nice tool.

As for the problem of ensuring "the player won't get repeats during the same game" - look here, especially the parts about "shuffling on demand".

##### Share on other sites
Zahlman,

Yours does look a lot cleaner. But when I tried it I got an error, something to do with a copy constructor and then it opens the ios_base.h file (line 668)
#include <iostream>#include <fstream>using namespace std;int getCount(ifstream input) {      string word;  int count = 0; // now the count of words is local, and will restart at 0  // each time the method is called. So now we can count multiple streams.  while(!input.eof()) {    input >> word;    count++;            }  return count;}int main() {  ifstream loadWords("words.txt");  if(!loadWords) {    cout << "\n\tError: The file words.txt cannot be found.\n"         << "\tPlease make sure that there is a file by that\n"         << "\tname in the same directory as this program.\n\n\t";    return 1;  }  cout << getCount(loadWords) << endl;  loadWords.close();}

Edit: fixed a source tag

##### Share on other sites
Quote:
 That's horrible.
Why thank you ;). Mind telling me why? The way I see it, general users of whatever the program is can just use it as they want, with the slightly more difficult job falling to those that want to batch script it. This makes sense, because you only have to write a batch script once.

Otherwise, how do you deal with a pause in the middle of the program? (note: you wouldn't just ignore a bunch of characters in the middle of a program). Sometimes these are needed, and since you don't like system("pause"), either a hack would need to be used, or command line options be written in.

##### Share on other sites
Quote:
 Original post by skulldrudgeryZahlman,Yours does look a lot cleaner. But when I tried it I got an error, something to do with a copy constructor and then it opens the ios_base.h file (line 668)*** Source Snippet Removed ***Edit: fixed a source tag

Oops. It needs to copy the ifstream to pass it by value, and apparently can't do that (and it would be silly anyway). Pass the ifstream by reference instead (not const; reading from it changes it):

int getCount(ifstream& input) {

Sorry :) (Man, I really should just download Dev-cpp so I can test the stuff I post ;) )