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 Jun 25 at 18:27
    
Not yet. Maybe in the next chapters of the book. –  morbidCode Jun 25 at 18:28
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 Jun 25 at 23:35

3 Answers 3

up vote 7 down vote accepted
  • Minor inconsistency here:

    #include<string>
    

    Add a space in 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 and try to keep it consistent.

  • Exception objects usually should not be passed by value:

    catch (domain_error e)
    

    They should be passed by const&:

    catch (domain_error const& e)
    
  • Consider separating this into additional lines:

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

    While it's okay to put short statements on a single line, it may be a little harder to read and maintain longer lines, also with the lack of curly braces.

    In addition, just call empty() instead of comparing size() to 0.

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

    string unique = v[0];
    

    use 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 Jun 25 at 18:21
    
In this case std::cin >> word would be the preferred method as it naturally reads space separated words. –  Loki Astari Jun 25 at 18:24
    
Ah, right. I forgot about that specific part in the problem. –  Jamal Jun 25 at 18:39

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 Jun 25 at 21:39

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::sort (std::begin (words), std::end (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
1  
You need to sort words before calling unique_copy, as that algorithm only omits consecutive copies. That is, with your code, the input "foo bar foo" counts 3 unique words. –  ruds Jul 12 at 5:09
    
@ruds Good catch. –  jliv902 Jul 14 at 12:50

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.