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\$

3 Answers 3

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\$
4
  • \$\begingroup\$ On the correct line but you can simplify. \$\endgroup\$ Commented Jun 7, 2012 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\$
    – Average
    Commented Jun 7, 2012 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\$ Commented Jun 7, 2012 at 16:09
  • \$\begingroup\$ @Jerry Coffin Yes I am using a new compiler, Thank you for your help! \$\endgroup\$
    – Average
    Commented Jun 7, 2012 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\$
7
  • \$\begingroup\$ I thought about using iterators, but that accesses elements at random right? \$\endgroup\$
    – Average
    Commented Jun 7, 2012 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\$ Commented Jun 7, 2012 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\$
    – Average
    Commented Jun 7, 2012 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\$
    – Average
    Commented Jun 7, 2012 at 22:51
  • \$\begingroup\$ Fixed the std::copy() above. You need the braces in this context. \$\endgroup\$ Commented Jun 8, 2012 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\$
3
  • \$\begingroup\$ You are right of course, but I think I will switch to using an iterator. \$\endgroup\$
    – Average
    Commented Jun 7, 2012 at 21:51
  • 1
    \$\begingroup\$ It's not about this case, it's about the general rule for loop coding. \$\endgroup\$
    – fogbit
    Commented Jun 8, 2012 at 7:27
  • \$\begingroup\$ You can just use auto. \$\endgroup\$ Commented Jun 28, 2016 at 9:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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