Search file for keyword program

Question

Just looking for some advice and I know using std namespace is bad but I didnt have the time to do all the using stuff. I want help on what here is good and what is bad practice :).

#include "stdafx.h"
#include <iostream>
#include <fstream>
#include <string>

using namespace std;

class file {
public:
    file() = default;
    file(const string &fileN) : fileName(fileN) { if (!fileName) { cout << "Invalid file closing" << endl; fileName.clear(); fileName.close(); } }

    bool setWord(const string &tWord);
    bool searchFile(const string&);
    ostream &output(ostream&) const;
private:
    ifstream fileName;
    string word;
    int occured;
};

bool file::setWord(const string &tWord) {
    if (!tWord.empty()) {
        word = tWord;
        return true;
    }
    else {
        cerr << "The word your trying to set is empty" << endl;
        return false;
    }
}

bool file::searchFile(const string &tWord) {
    if (setWord(tWord)) {
        string temp;
        occured = 0;
        while (fileName >> temp) {
            if (temp == word) {
                ++occured;
            }
        }
        return true;
    }
    return false;
}

ostream &file::output(ostream &os) const {
    os << word << " occured " << occured << " times in your file";
    return os;
}

int main()
{
    file test("stdafx.h");
    if (test.searchFile("hey"))
        test.output(cout) << endl;
    return 0;
}

Answer 1

If I understand correctly, the program counts the occurrences of some words in a file. I have the following observations:

Error handling

I would recommend throwing exceptions instead of/additionally to writing to cout and cerr (btw., I think if you do write to these streams, you should stick to one of them, most probably cerr).

EDIT: Note, that as the constructor is written currently, we can end up having an object in an invalid state after calling it. This would be prevented by throwing an exception.

Encapsulation, state of the class

I suggest making setWord private, or at least protected. As it is now, one could call:

file test("stdafx.h");
test.searchFile("hey");
test.setWord("fooBar");
test.output(cout) << endl;

Also note, that currently it is possible that a client calls test.output straight away:

file test("stdafx.h");
test.output(cout) << endl;

In this case, he would probably get meaningless results. I therefore suggest introducing a check at the beginning of the implementation of the output method (if you need it at all, see below), and e.g. throw an exception if no search was performed yet.

Constructors

In this particular case, I would suggest initialising fileName only in the body of the constructor, after checking the validity of the parameter. In other words, I would first validate, and only then initialise, instead of initialising with a (possibly invalid) parameter and then cleaning up.

Also, it is not clear to me what the purpose of the default constructor is. Would an object constructed through that constructor work correctly? If not, then you can probably remove it, and keep only the parametrised one.

Destructors

I recommend adding a destructor to this class, where the file stream opened in the constructor is closed again.

Idempotency

Note, that currently it is possible to call searchFile only once. After the first call, we will be at the end of the input stream, and thus it is not possible to read from it anymore. I therefore recommend to either reset the stream (or close/open it again), or introduce a similar check as mentioned above for the state of the object (i.e., throw an exception if searchFile is called twice).

Responsibility of the class

Instead of the method ostream &output(ostream&) const;, I would rather introduce a method int getWordCount() const, which would return the actual count, and then leave it up to the client how/if he wants to print this count (of course this would also need the above mentioned check, whether the object is in the right state).

Maybe it would be even better to have searchFile return the count directly (and throw an exception if the word we are looking for is empty, instead of communicating this fact through the return value).

Naming

I recommend changing the name of the variable fileName to something which better reflects what it is, e.g. fileStream. (fileName makes me think of a string holding the name of the file, not a stream through which the contents can be read).

Code blocks

I suggest always adding braces around code blocks, e.g. for the code above:

if (test.searchFile("hey")) {
    test.output(cout) << endl;
}

A modified version

I have modified the original code to demonstrate usage of exceptions, and include some of the other suggested changes. Note, that by removing some of the methods most of the "illegal state" problems can be fixed as well. I did not address the "idempotency" issue.

// search.cpp

//#include "stdafx.h"
#include <iostream>
#include <fstream>
#include <string>

using namespace std;

class FileNotFoundException {};
class IllegalArgumentException {};


class file {
public:
    file(const string &fileN) : fileStream(fileN.c_str()) {
       if (!fileStream) {
          cerr << "Invalid file closing" << endl;
          throw FileNotFoundException();
       }
    }
    ~file() {
       if (fileStream) {
          fileStream.close();
       }
    }

    int searchFile(const string&);
private:
    ifstream fileStream;
};

int file::searchFile(const string &tWord) {
    if (tWord.empty()) {
       cerr << "Word is empty" << endl;
       throw IllegalArgumentException();
    }
    string temp;
    int occured = 0;
    while (fileStream >> temp) {
        if (temp == tWord) {
            ++occured;
        }
    }
    return occured;
}

int main(int argc, char * argv [])
{
    try {
        file test("stdafx.h");   // does not exist on my system
    }
    catch (const FileNotFoundException &) {
        cerr << "FileNotFoundException caught!" << endl;
    }

    file test2("search.cpp");  // it is this file
    const int foundCnt = test2.searchFile("file");

    cout << "Found " << foundCnt << " occurrences. " << endl;

    file test3("search.cpp");
    try {
        const int foundCnt2 = test3.searchFile("");
    }
    catch (IllegalArgumentException &) {
        cerr << "IllegalArgumentException caught!" << endl;
    }

    return 0;
}

Note that by having the constructor throwing an exception if the file name is invalid, we prevent the file object from being used in an invalid state. The reason is, that (in our example above), we cannot use the object test after this line:

  file test("stdafx.h");   // does not exist on my system