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.

I'm extremely new to C++ and am doing the exercises on the book Accelerated C++. Here is one of the exercises:

4-5. Write a function that reads words from an input stream and stores them in a vector. Use that function both to write programs that count the number of words in the input, and to count how many times each word occurred.

How can I make this program better by only using vectors?

#include <iostream>
#include <algorithm>
#include <vector>
#include<string>
#include <stdexcept>
using std::cout; using std::cin; using std::vector; using std::sort; using std::string; using std::endl; using std::domain_error; using std::cerr;

//it said "count the number of words in the input, and to count how many times each word occurred". I cannot think of any return value if a function do two things, so I used void.
void run(const vector<string>& v) {
    if (v.size() == 0) throw domain_error("Nothing to count");
    cout << "words count: " << v.size() << endl;
    string unique = v[0];
    int count = 1;
    for (vector<string>::size_type i = 1; i < v.size(); ++i) {
        if (v[i-1] == v[i]) {
            count++;
        } else {
            cout << unique << ": " << count << endl;
            unique = v[i];
            count = 1;
        }
    }
    cout << unique << ": " << count << endl;
}

int main() {
    cout << "Enter all words followed by end of file." << endl;
    vector<string> words;
    string word;
    while (cin >> word) {
        words.push_back(word);
    }
    sort(words.begin(), words.end());
    try {
        run(words);
    } catch (domain_error e) {
        std::cerr << e.what() << endl;
    }
    return 0;
}
share|improve this question
    
Have you been tought how to use a std::map<> yet? If so that makes counting the words trivial. –  Loki Astari yesterday
    
Not yet. Maybe in the next chapters of the book. –  user3263252 yesterday
1  
An empty vector means simply that there were no words in the input. It shouldn't trigger an exception; the run function should be able to deal with it appropriately. –  Pete Becker yesterday
add comment

3 Answers

up vote 4 down vote accepted
  • Minor inconsistency here:

    #include<string>
    

    Add a space between them, like all the others:

    #include <string>
    
  • Just get rid of this:

    using std::cout; using std::cin; using std::vector; using std::sort; using std::string; using std::endl; using std::domain_error; using std::cerr;
    

    Even if you put them onto separate lines, it'll still be lengthy. Just put std:: where necessary.

  • Exception objects should not be passed by value:

    catch (domain_error e)
    

    They should be passed by const&:

    catch (domain_error const& e)
    
  • Consider separating this out a bit:

    if (v.size() == 0) throw domain_error("Nothing to count");
    

    While it's okay to put such statements on a single line, it may also be hard to see clearly with no newlines in between. You could still use curly braces, which also improves maintainability.

    In addition, just call std::vector::empty() instead of comparing the size to 0.

    if (v.empty())
    {
        throw domain_error("Nothing to count");
    }
    
  • Instead of getting the first element via []:

    string unique = v[0];
    

    use std::vector::front():

    string unique = v.front();
    

I'd also like to add a compliment:

Thank you for using std::size_type instead of int. I hardly see that done by others. It's good to see that you're paying attention to return types and compiler mismatch warnings.

share|improve this answer
    
istream::operator>>(string) is quite different std::getline(istream&, string&). The problem calls for "reading words from an input stream", so istream::operator>>(string) seems more appropriate. –  200_success yesterday
    
In this case std::cin >> word would be the preferred method as it naturally reads space separated words. –  Loki Astari yesterday
    
Ah, right. I forgot about that specific part in the problem. –  Jamal yesterday
add comment

Everything said by @Jamal

In addition I think you are counting the words in a hard way.

If you were using a std::map<> counting becomes real easy. But we will leave that solution to next month (when you read the next chapter).

But you should still become aware of the functions available in the standard library.

This function finds the range of iterators that match a specific value:

template <class ForwardIterator, class T>
pair<ForwardIterator,ForwardIterator>
equal_range (ForwardIterator first, ForwardIterator last, const T& val);

This function calculates the distance between two iterators:

template< class InputIt >
typename std::iterator_traits<InputIt>::difference_type
distance( InputIt first, InputIt last );

// Not always efficient. But for std::vector iterators it is very
// efficient being a simple subtraction.

If we plug this into your code:

sort(words.begin(), words.end());
for(auto rangeStart = words.begin(); rangeStart != words.end(); )
{
     auto wordGroup = equal_range(rangeStart, words.end(), *rangeStart);
     std::cout << *rangeStart << ": " << std::distance(wordGroup.first, wordGroup.last) << "\n";

     rangeStart = wordGroup.second;
}

This has the characteristics of \$O(nlog(n))\$ which is better than the \$O(n^2)\$ suggested by counting each of the words (as suggested by @liv902).

share|improve this answer
    
+1 I did not know there was an std::equal_range. You should modify the "(suggested above)" since the ordering of the posts may vary from user to user (votes, activity, oldest). –  jliv902 yesterday
add comment

Since you used std::sort(), I assume you can use other functions from the standard library. Using the standard library functions, you entire program can be simplified to this:

#include <algorithm>
#include <iostream>
#include <iterator>
#include <string>
#include <vector>

int main (void)
{
    std::vector <std::string> words ;

    std::copy (
        std::istream_iterator <std::string> (std::cin),
        std::istream_iterator <std::string> (),
        std::back_inserter (words)
    ) ;

    std::vector <std::string> unique_words ;
    std::unique_copy (
        std::begin (words), 
        std::end (words), 
        std::back_inserter (unique_words)
    ) ;

    std::cout << "Total number of words: " << words.size () << "." "\n" ;
    std::cout << "Total number of unique words: " << unique_words.size () << "." "\n" ;

    for (const std::string &s : unique_words) {
        std::cout << s << ": " << std::count (std::begin (words), std::end (words), s) << "\n" ;
    }

    return 0 ;
}

Please note that I used a few C++11 features that may or may not be available to your compiler. std::begin() and std::end() can be replaced with words.begin() and words.end(). The range-based for-loop could be replaced with a normal for-loop.

One downside of doing it this way is that the std::count part of this code would be inefficient. If performance matters, you could sort the std::vector and create a function that would count adjacent words and return an iterator to the next set of words.

Some other things I noticed:

  1. Give your run() function a more meaningful name.
  2. Consider whether you really need to throw an exception. If this function performed something critical that needed to happen, then I could see the justification of throwing an exception. But in this case, I think outputting nothing or outputting something like "vector is empty" would be fine.
  3. Avoid multiple statements on a single line.

    if (v.size() == 0) throw domain_error("Nothing to count");
    

    If you are writing an inline function within a class definition, then I guess that would be okay. I prefer using braces everywhere, but at least do this:

    if (v.size() == 0) 
        throw domain_error("Nothing to count");
    
  4. I know that you are constrained to using only std::vector for this problem, but in the real world, always try to think about the best data structure for the job.
share|improve this answer
add comment

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.