Yes this can be simplified to:
int main()
{
std::ifstream inputFile("Bob");
std::unordered_map<std::string, int> count;
std::for_each(std::istream_iterator<std::string>(inputFile),
std::istream_iterator<std::string>(),
[&count](std::string const& word){++count[word];});
}
Why this works:
operator>>
When you read a string from a stream with operator>> it read a space separated word. Try it.
int main()
{
std::string line;
std::cin >> line;
std::cout << line << "\n";
}
If you run that and type a line of text. It will only print out the first space separated word.
std::istream_iterator
The standard provides an iterator for streams. std::istream_iterator<X> will read an object of type X from the stream using operator>>.
This allows you to use streams just like you would any other container when using standard algorithms. The standard algorithms take two iterators to represent a container (begin and end or potentially any two points in the container).
So by using std::istream_iterator<std::string> you can treat a stream like a container of space separated words and use it in an algorithm.
int main()
{
std::string line;
std::istream_iterator<std::string> iterator(std::cin);
line = *iterator; // de-reference the iterator.
// Which reads the stream with operator >>
std::cout << line << "\n";
}
std::for_each
I use std::for_each above because it is trivial to use. But with a tiny bit of work you can use the range based for loop introduced in C++11 (as this just calls std::begin, std::end on the object to get the bounds of the loop.
But lets look at std::for_each first.
std::for_each(begin, end, action);
Basically it loops from begin to end and performs action on the result of de-referencing the iterator.
// In my case action was a lambda
[&count](std::string const& word){++count[word];}
It captures count from the current context to be used in the funtion. And de-referencing the std::istream_iterator<std::string> returns a reference to a std::string object. So we can not use that to increment the count for each word.
Note: count is std::unordered_map so be looking up a value it will automatically insert it if it does not already exist (using default value (for int that is zero). Then increment that value in the map.
Range based for
A quick search to use range based for with std::istream_iterator gives me this:
template <typename T>
struct irange
{
irange(std::istream& in): d_in(in) {}
std::istream& d_in;
};
template <typename T>
std::istream_iterator<T> begin(irange<T> r) {
return std::istream_iterator<T>(r.d_in);
}
template <typename T>
std::istream_iterator<T> end(irange<T>) {
return std::istream_iterator<T>();
}
int main()
{
std::ifstream inputFile("Bob");
std::unordered_map<std::string, int> count;
std::for(auto const& word : irange<std::string>(inputFle)) {
++count[word];
}
}
Issues with this technique.
We use space to separate words. So any punctuation is going to screw things up. Not to worry. C++ allows you to define what is a space in any given context. So we just need to tell the stream what is a space.
https://stackoverflow.com/a/6154217/14065
Review of code
Sure.
struct StringOccurrence //stores word and number of occurrences
{
std::string m_str;
unsigned int m_count;
StringOccurrence(const char* str, unsigned int count) : m_str(str), m_count(count) {};
};
But you can do this with a number of standard types.
typedef std::pair<std::string, unsigned int> StringOccurrence;
You are doing this to store the value in a vector. But a better way to store this is in a map. Because maps are ordered in some way internally lookup is a lot faster. std::map gives access in O(ln(n)) or std::unordered_map gives access in O(1).
I hate bad comments.
Bad comments are worse than no comments because they need to be maintained and the compiler will not help you maintain them.
if (!in) //check if file path is valid
Not quite, but close enough I suppose. But I don't really need the comment to tell me that. The code seems pretty self explanatory.
Note sure if -1 is a good value. It will really depend on the OS you are running on. 0 is the only valid value. Anything else is considered an error. At your OS level this will probably be truncated to 255 on most systems (but not all).
return -1;
If you run this:
> cat xrt.cpp
int main()
{
return -1;
}
> g++ xrt.cpp
> ./a.out
> echo $? # Echos the error code of the last command.
255
I don't think you need to copy the whole thing into memory.
std::vector<std::string>vec;
std::string lineBuff;
while (std::getline(in, lineBuff)) // write multiline text to vector of strings
{
vec.push_back(lineBuff);
}
Just read a line at a time and processes that.
Don't use pointers in C++
std::vector<StringOccurrence*> strOc;
C++ has much better ways to handle dynamic memory allocation and pointers is never the way to go.
When you iterate from begin -> end of something. You can use the new range based for instead.
for (auto it = vec.begin(); it < vec.end(); it++)
// easier to write and read:
for(auto const& val : vec)
Going to comment on your comments again.
for (auto it = vec.begin(); it < vec.end(); it++) //itterate through each line
Not very useful. I can see that you are iterating over every line. From the code.
You should restrict your comments to WHY you are doing something.
Space ' ' is not the only white space character! What about tab or carrige return \r or vertical tab \v. You should test for space using standard library routines.
std::is_space(c)
I have use goto probably twice in the last ten years. One of those times was probably wrong.
goto end; //skip next step (need fix?)
Loops and conditions will always be better and easier to read.
We have a leak her:
strOc.push_back(new StringOccurrence(stringBuff.c_str(), 1));
I see a new (but no delete). See above about using pointers. There is no need to use a pointer here. Just use a normal object it will be moved into the vector.
