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