5
\$\begingroup\$

I am working through the exercises in the book Accelerated C++. I wanted to get some feedback on proper use of the Standard Library, correctness, overall style, portability, etc.

/*******************************************************************************
Accelerated C++ Chapter 3

Write a program to count how many times each distinct word appears in its input
*******************************************************************************/

#include <iostream>
#include <vector>
#include <string>

int main()
{
    std::cout << "Enter a list of words followed by end-of-file: ";

    typedef std::vector<std::string>::size_type vector_sz;
    std::vector<std::string> words;
    std::string word;
    while (std::cin >> word)
    {
        words.push_back(word);
    }

    vector_sz size = words.size();
    if(size == 0)
    {
        std::cout << "Please enter some words!";
        return -1;
    }

    std::vector<int> frequency;
    std::vector<std::string> unique_words;
    int count = 0;
    bool isUnique;

    unique_words.push_back(words[0]);

    for(unsigned i = 0; i < words.size(); ++i)
    {
        isUnique = true;

        for(unsigned j = 0; j < unique_words.size(); ++j)
        {
            if(words[i] == unique_words[j])
            {
                isUnique = false;
            }
        }
      if(isUnique)
      {
          unique_words.push_back(words[i]);
      }
    }

    for(unsigned i = 0; i < unique_words.size(); ++i)
    {
        for(unsigned j = 0; j < words.size(); ++j)
        {
            if(unique_words[i] == words[j])
            {
                count += 1;
            }
        }
        frequency.push_back(count);
        count = 0;
    }

    for(unsigned i = 0; i < unique_words.size(); ++i)
    {
        std::cout << unique_words[i] << "   " << frequency[i] << std::endl;
    }
    return 0;
}
\$\endgroup\$
6
\$\begingroup\$

You could cut your code in half if you used a map:

typedef   std::map<std::string, unsigned int> Map;

Map tMap;
while(std::cin >> word)
{
    // When accessing a map with `[]` If the value does not exist
    // it is created and initialized to zero.
    //
    // The plus plus then increments the stored value
    tMap[word]++;
}

// Iterate through map and print frequency

Couple other notes from the code you have:

  • After unique_words.push_back(words[0]); you can start the i loop at index 1 instead of 0.
  • When checking for uniqueness in the first pair of loops, once you determine uniqueness to be false you can stop looping for that word: while(j < unique_words.size() && isUnique) ...
\$\endgroup\$
  • \$\begingroup\$ On the correct line but you can simplify. \$\endgroup\$ – Martin York Jun 7 '12 at 4:26
  • \$\begingroup\$ At first glance, I don't quite understand how this works, but I will research how to use a map, thank you! \$\endgroup\$ – newToProgramming Jun 7 '12 at 11:38
  • \$\begingroup\$ Assuming a new compiler and no requirement to print the results in order, you might also consider std::unordered_map instead of std::map. Also, though it makes no difference here, absent a reason to do otherwise, you generally want to use pre-increment instead of post-increment (++tMap[word];). \$\endgroup\$ – Jerry Coffin Jun 7 '12 at 16:09
  • \$\begingroup\$ @Jerry Coffin Yes I am using a new compiler, Thank you for your help! \$\endgroup\$ – newToProgramming Jun 7 '12 at 21:44
6
\$\begingroup\$

See @sfpiano about using a map directly to count each word.

This is perfectly fine.

    std::vector<std::string> words;
    std::string word;
    while (std::cin >> word)
    {
        words.push_back(word);
    }

But if you want to learn the STL then you can replace the above with a single line:

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

You don't need to store the value (especially since you don't re-use it)

    vector_sz size = words.size();
    if(size == 0)

Do this in a single line:

    if (words.size() == 0)

But this test is embodied in a single call.

    if (words.empty())

Here you use two vectors.

    std::vector<int> frequency;
    std::vector<std::string> unique_words;

This can be replaced by a std::map (see sfpiano) for details.

Declare as close to the use point as possible.

    bool isUnique;

This is not used outside the loop. So declare it inside the loop (and initialize at declaration.

    // So it should look like this.
    for(unsigned i = 0; i < words.size(); ++i)
    {
        bool isUnique = true;

Also you should start using iterators rather than using the index.

    for(auto loop = words.begin(); loop != words.end(); ++loop)
\$\endgroup\$
  • \$\begingroup\$ I thought about using iterators, but that accesses elements at random right? \$\endgroup\$ – newToProgramming Jun 7 '12 at 11:39
  • \$\begingroup\$ @newToProgramming: No. For vectors. begin() is index 0 and ++ moved through the vector in order. For Map/Set the order is the sorted order of the container. \$\endgroup\$ – Martin York Jun 7 '12 at 15:21
  • \$\begingroup\$ I am having trouble implementing std::copy from your code snippet, but I will figure it out, thank you for all of the corrections, I clearly have a lot to learn about using c++ correctly. Thanks. \$\endgroup\$ – newToProgramming Jun 7 '12 at 22:33
  • \$\begingroup\$ ok, i figured it out, the second argument in std::copy is initialized with the istream_iterator default constructor which points to end-of-file, so it should be istream_iterator<std::string>() or just omit the paranthesis entirely. Is that correct? \$\endgroup\$ – newToProgramming Jun 7 '12 at 22:51
  • \$\begingroup\$ Fixed the std::copy() above. You need the braces in this context. \$\endgroup\$ – Martin York Jun 8 '12 at 3:37
3
\$\begingroup\$

As long as you've typedef'ed std::vector<std::string>::size_type as vector_sz, use it in the code

for(vector_sz i = 0; i < words.size(); ++i)
\$\endgroup\$
  • \$\begingroup\$ You are right of course, but I think I will switch to using an iterator. \$\endgroup\$ – newToProgramming Jun 7 '12 at 21:51
  • 1
    \$\begingroup\$ It's not about this case, it's about the general rule for loop coding. \$\endgroup\$ – fogbit Jun 8 '12 at 7:27
  • \$\begingroup\$ You can just use auto. \$\endgroup\$ – Archie Gertsman Jun 28 '16 at 9:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.