Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Is this a good approach or is there some other solution that I am not aware of?

//C++ program to count number of words in text file

#include<fstream>
#include<iostream>
#include<string>
using namespace std;

int main()
{
    ifstream inFile; //Declares a file stream object
    string fileName;
    string word;
    int count = 0;

    cout << "Please enter the file name ";
    getline(cin,fileName);

    inFile.open(fileName.c_str());

    while(!inFile.eof())
    {               
        inFile >> word; 
        count++;
    }

    cout << "Number of words in file is " << count;
    inFile.close();

    cin.get();  
    return 0;
}
share|improve this question
2  
Please do add some comment, like what your class have to do. Are there some specific things we need to review? –  chillworld May 6 at 6:11
    
I want to ask, is this the good approach or there is some other solution that I am not aware of? –  Deepak Singh May 6 at 6:28

4 Answers 4

up vote 12 down vote accepted

Your code has a few problems.

  1. You should learn to not use using namespace std;. It's generally frowned upon.
  2. You should never use while(!inFile.eof()). It's pretty much a guaranteed bug.
  3. You should use standard algorithms when they're applicable (as they are here).
  4. Prefer to fully initialize variables at creation (e.g., pass file name when you create a filestream object).
  5. Generally let the destructor handle destruction (e.g., let the filestream object close when it goes out of scope)1.

I'd also strongly prefer to take command line arguments over prompting for input at run time.

I'd write the code using the standard distance algorithm, something like this:

#include <fstream>
#include <algorithm>
#include <iterator>
#include <iostream>
#include <string>
#include <cstdlib>

int main(int argc, char **argv) {
    if (argc < 2) {
        std::cerr << "Usage: count_words <infile>\n";
        return EXIT_FAILURE;
    }

    std::ifstream infile(argv[1]);

    std::istream_iterator<std::string> in{ infile }, end;

    std::cout << "Word count: " << std::distance(in, end);
}

  1. There are a few cases where it makes sense to manually close a file. For example, if you're moving a file between file systems by copying its content, then deleting the original, you want to do everything you can to ensure the copy completed successfully (including successful closing) before you remove the original. Anything that might destroy the user's data calls for extraordinary measures to assure safety. This isn't one of those cases though.
share|improve this answer
    
I wouldn't put two variables in one line (like you did with in and end. –  BЈовић May 6 at 7:49
    
Darn you. That's a nice easy question. The one thing I would add (which is not worth a separate answer). Is not to bother open file on a separate line (ie constructor) or manually close a file (ie dstructor. rely on RAII). –  Loki Astari May 7 at 1:15
    
Very minor point, but I believe EXIT_FAILURE is defined in <cstdlib> which is not included. –  Edward May 7 at 1:59
    
@Edward: I believe you're correct (or were correct, anyway). –  Jerry Coffin May 7 at 2:02

using namespace std; is a bad habit.


Declare variables as close to use as possible. (for example, declare count about the while loop rather than at the top).


An eof loop control does not work the way you think it does. eof is not reached until after the end of file is attempted to be read past. This means that your last read can silently fail.

The more natural way to write that loop is:

while (inFile >> word) { ++count; }

I would put a newline after the count output. It's rare to have a program output without a trailing newline. (Although I'm only familiar with linux--maybe no break is normal under Windows.)


Rather than using cin.get(), I would just run the application inside of a console. It's a bit of a strange behavior to have the program hang until you hit a key. Imagine if programs like grep, cat or wc did that. It would be a pain to use.


Since you're only using one parameter and there's no real advantage to using a user prompt, I would take the filename as an argument to the program rather than reading it from the console. (In other words, I would use argv.)


If a program can't fail, it's fairly common to omit a return code. That clearly signals that the program can't fail. (Note: main is a special case. Return values are always required in other functions.)


If you wanted to leverage the standard library, you could actually do this much shorter:

ifstream fs(...);
std::size_t count = std::distance(std::istream_iterator<std::string>(fs), 
                                  std::istream_iterator<std::string>());

The specifics of this are a bit advanced, but the basic idea is that istream_iterator is a simple wrapper around operator>> and that it performs extractions until it no longer can. Since std::distance runs the length of the first iterator until the second, this will just read as many tokens as it can and return the distance (i.e. count). (Technically it doesn't always actually iterate along the iterators. When it can, it will just do a simple subtraction. That's irrelevant here though.)


I wouldn't bother closing the file. Unless you plan on doing actual error handling, it's best to just let the scope of the file handle the close of it. When it goes out of scope, the destructor will run which will close the file if it's still open.

share|improve this answer

Plus what ppl mentioned, you need to double check if the file is opened successfully otherwise you should not manipulate it.

if ( inFile.fail() )
{
  // do something
  return -1;
}
share|improve this answer
1  
It's typically a bad idea to return negative values. The only values guaranteed to be meaningful are EXIT_SUCCESS and EXIT_FAILURE, but more importantly, certain systems (like linux) use unsigned numbers to represent exit statuses, so that -1 silently becomes a large positive number. It doesn't matter as long you're only checking code == 0 or not, but considering there's no advantage over EXIT_FAILURE, there's no real reason to use anything else. –  Corbin May 7 at 19:39

It's better to put using namespace inside your code block (main()) or don't use it. Otherwise it will generate some namespace conflicts when you write a large project.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.