Simple 'for' loop crashing program on final iteration

Question

The below program seems to be crashing at the very end each time, and I'm assuming that this is because once I get to i = (size-1) then wordList[i+1] won't return anything, returns null, or something equivalent. Any way around this? Am I correct that this is the source of my problem?

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

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

// Obtain a list of words and return the de-duped list
// with an accompanying word count
int main()
{
    cout << "Enter a series of words separated by spaces, "
            "followed by end-of-file: ";

    vector<string> wordList;
    string x;
    while (cin >> x)
          wordList.push_back(x);

    typedef vector<string>::size_type vec_sz;
    vec_sz size = wordList.size();
    if (size == 0) {
       cout << endl << "This list appears empty.  "
                       "Please try again."  << endl;
       return 1;
    }

    sort(wordList.begin(), wordList.end());

    cout << "Your word count is as follows:" << endl;
    int wordCount = 1;
    for (int i = 0; i != size; i++) {
        if (wordList[i] == wordList[i+1]) {
           wordCount++;
           }
        else {
             cout << wordList[i] << "    " << wordCount << endl;
             wordCount = 1;
             }
         }
    return 0;
}

Answer 1

You may want to try

for (int i = 0; i < size; i++)

This will stop the loop from trying to address an index that is out of bounds.

Answer 2

Yes. Boundary condition fails. vector[size()] is undefined value.

  if (wordList[i] == wordList[i+1]) ---> out of bounds

Work around could be iterate size()-1

Answer 3

That is exactly the problem. Your vector contains "size" items, but you're trying to access past the end of the vector.

What you need to do is stop the for loop before the last item, so that i+1 only accesses the last item.

For example:

for (int i = 0; i < (size - 1); i++) {

Answer 4

Change

for (int i = 0; i != size; i++) {

to

for (int i = 0; i != size-1; i++) {

Answer 5

That's because for i == size-1, i + 1 == size. And wordList[size] is off the end of your list. You probably need to change your for loop condition to "i < size - 1".

Answer 6

You should change your code to for (int i = 0; i < size-1; i++) { because in wordList[i+1] value of i+1 can must be less then size. Which in result means i < size -1.

Is C/C++ arrays can have indexes from 0 to size-1.

Answer 7

It's not NULL, it's outside the range. You should at least be checking if i < size before you do that comparison.

I'm not sure what you're trying to do with the comparison anyway, but that's a broader question.

Answer 8

Two problems.

First, it's generally better to loop till your index is < your size, instead of using !=. What you have should still work here though.

The problem is this line:

if (wordList[i] == wordList[i+1])

You need to either stop your loop one earlier (i < size - 1) OR change your if statement so that it only checks wordList[i] == wordList[i+1] when there is at least one more entry in the array.

An easier way might be to loop from i = 1 to i < size, and check wordList[i-1] == wordList[i]:

for(int i = 1; i < size; i++) {
  if(wordList[i - 1] == wordList[i]) {
    /* same as before */
  }
  /* everything else the same */
}

That will prevent you from needing extra if statements but will still protect you from leaving the bounds of the array.

EDIT:

As mentioned in the comments, if you use all the original code and just change the loop like I mentioned there will be an off-by-1 error, but it's easy to fix.

Start the word count at 1 instead of 0. You know that you'll always have at least one unique word. The loop will increment that count any time it sees a new word, so you'll end up with the proper count.

The only extra handling you'll need is for when the wordList is empty. So you'll need one quick check to see if the word list is empty, and in that case the count is 0.

Answer 9

You could do a check for i == wordList length and exit out/skip the if.

Answer 10

You need two changes. As stated in previous comments, your for ending condition should be < size-1 to prevent reading out of bounds. You also need to print the count of the last item, which will be wordCount because if it's unique wordCount is 1 and if not it has been already added. Corrected code:

for (int i = 0; i < size-1; i++) {
    if (wordList[i] == wordList[i+1]) {
       wordCount++;
       }
    else {
         cout << wordList[i] << "    " << wordCount << endl;
         wordCount = 1;
         }
     }
cout << wordList[size-1] << "    " << wordCount << endl;

Answer 11

Sometimes it's just that the code could be much simpler... if you used something else.

Problem statement: return a de-duped list of words with a count associated to each word.

Personally I would go for a std::map for such a statement.

// Yeah it feels stupid, but I want default construction to initialize...
struct MyCount
{
  MyCount() : _count(0) {}
  size_t _count;
};

std::ostream& operator<<(std::ostream& out, const MyCount& rhs)
{ 
  return out << rhs._count;
}

int main()
{
  cout << "Enter a series of words separated by spaces, "
          "followed by end-of-file: ";

  typedef std::map<std::string, MyCount> map_type;
  map_type wordList;
  std::string x;
  while (std::cin >> x)
    wordList[x] += 1; // This is why I have a struct instead of a plain size_t

  if (wordList.empty()) {
    cout << endl << "This list appears empty.  "
                    "Please try again."  << endl;
    return 1;
  }

  cout << "Your word count is as follows:" << endl;
  for (map_type::const_iterator it = wordList.begin(), end = wordList.end(); it != end; ++ it) {
    std::cout << it->first << "    " << it->second << std::endl;
  }
  return 0;
}

I know that std::vector is the default container, but when if you are 'associating' two entities (here a word and its number of occurrences), you'll be better off thinking Associative Container.