Trouble with vectors of structs: derefrencing iterators (probably easy)

Question

I'm having trouble with the program. The program is intended to take input from a file in the form of names separated by a comma, and tally them in an output file (which I haven't gotten to yet). It fails before "good so far 3". Apparently, what it's breaking on is defrencing the iterator it. That is to say, what I'm trying to do is use it as a pointer, and ++ that pointer. This should skip ahead to the next struct in the array. Then, I want to use it->inputName to access something inside of the struct. However, when I do it->inputName, it tries to use it as a pointer and it can't do that. Not sure where to go from here, if I try to make a regular pointer like name * it, it doesn't work with nameStorage.begin(), as that only takes an iterator. I'm on windows 7 using Microsoft Visual Studio 2010, if that helps.

I'm fairly new to programming, so any other tips would also be great. Thank you!

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

using namespace std;

struct name{
    int tally;
    string inputName;
};

bool die( const string& message );

int main( void ) {
    ifstream infile;
    infile.open( "input.txt", ifstream::in );

    vector<name> nameStorage; // stores all names and tallies of them
    vector<name>::iterator it;
    string tempInput; // Stores the most recent input temporarily
    char temp;
    int counter = 0;
    while ( temp = infile.get(), infile.good() ) { // loop while extraction from file is possible
        if ( temp != ',' ) {
            tolower ( temp ); // makes everything lowercase
            tempInput.push_back(temp); }
        else {
            cout<<"good so far"<<endl;
            for ( it = nameStorage.begin(); it <= nameStorage.end(); it++ ) {
                cout<<"good so far 2"<<endl;
                if ( tempInput == it->inputName ) { 
                    cout<<"good so far 3"<<endl;
                    it->tally++;
                    break;
                }
                else if ( it == nameStorage.end() ) {
                    name tempStruct;
                    tempStruct.inputName = tempInput;
                    tempStruct.tally = 1;
                    nameStorage.push_back( tempStruct );
                }
            }
            tempInput.clear(); // clears the string after it has stored it
        }
        counter++;
        cout<<"Detected "<<counter<<" characters so far."<<endl;
    }
}

bool die( const string& message ) {
    cerr<<message;
    exit (EXIT_FAILURE);
}

Answer 1

Adding another answer to cover questions about maps and std::getline

A map is an associative container that associates a key with a value. For example, std::map is a map with a key of type std::string, and a value of type int. Maps support fast lookups of a key (log(n)). eg.

std::map<std::string, int> nameCount;

Is a map that will store a int value associated with a string key. (See code example after explanation of std::getline).

std::getline treats the specified delimiter as the end of line character. The usual way of using it is to first get the whole line delimited by EOL, then get lines from that line using the delimiter e.g. ','

Here is some code that achieves what you want.

 std::map<std::string, int> nameCount;
 std::ifstream myfile(filename.c_str(),std::ios::in);
 std::string line;
 while(std::getline(myfile, line))
 {
    std::stringstream linestream(line);
    std::string       name;
    while (std::getline(linestream, name, ','))
    {
      if (nameCount.find(name) != nameCount.end())
      {
        nameCount[name]++; // increases the tally for name
      }
      else
      {
        nameCount[name] = 1; // inserts name into map and sets the tally to 1
      }
    }
 }
 // now output the map
 std::map<std::string, int>::iterator namecountitr;
 for (namecountitr = nameCount.begin(); 
       namecountitr != nameCount.end(); namecountitr++)
 {
      std::cout << namecountitr->first.c_str() << ":" 
      << namecountitr->second << std::endl;
 }

Enjoy.

Answer 2

This code has at least 4 problems with it, not least of which is that using vectors for this kind of lookup/tally is extremely inefficient.

1) std::vector::end() returns a special type of iterator that does not support boolean operators such as <= (but does support operator- or operator-=). Well, it supports them, but the behaviour is undefined.

So

for ( it = nameStorage.begin(); it <= nameStorage.end(); it++ )

should be

for ( it = nameStorage.begin(); it != nameStorage.end(); it++ )

2) Now that your for statement is correct, this comparison will never return true

else if ( it == nameStorage.end() ) {

and so your new value will never be stored in the vector. To find tempInput in nameStorage you can either use std::find

if (std::find(nameStorage.begin(), nameStorage.end(), tempInput) != nameStorage.end())
{
 /// temp struct blah blah blah
 nameStorage.push_back(tempStruct);
}

3) Using boolean equivalence operator on strings is generally considered bad form i.e.

if ( tempInput == it->inputName ) {

should be

if (!tempInput.compare(it->InputName))

but you wont need to do that if you use std::find (above).

4) std::getline supports delimiters, and you should be using it instead of reading in 1 char at a time see http://www.cplusplus.com/reference/string/getline/

Last, you should really be doing your lookup/tally with a map. std:map is fine

std::map<std::string, int> nameStorage;

if (nameStorage.find(tempInput) != nameStorage.end())
{
 nameStorage[tempInput]++;
} else
{
 nameStorage[tempInput] =1;
}

Answer 3

Change:

for ( it = nameStorage.begin(); it <= nameStorage.end(); it++ )

To:

for ( it = nameStorage.begin(); it != nameStorage.end(); it++ )

Move:

else if ( it == nameStorage.end() ) {
          name tempStruct;
          tempStruct.inputName = tempInput;
          tempStruct.tally = 1;
          nameStorage.push_back( tempStruct );
        }

Out of your now changed for loop, you don't need the else if since this is the condition for the for loop end:

name tempStruct;
tempStruct.inputName = tempInput;
tempStruct.tally = 1;
nameStorage.push_back( tempStruct );

Check this simple example here.

Answer 4

The range of a vector is [ begin(), end() ), end() isn't included in that range of valid elements. end() refers to an element (if you can call it that) past the end of the vector.

Therefore you're obviously going to be having a problem dereferencing the iterator when it == nameStorage.end(), since it won't be referring to an existing element:

if ( tempInput == it->inputName )

The convention is to terminate on != end(), which will eliminate the off-by-one problem above:

for ( it = X.begin(); it != X.end(); it++ )

The idea of iterators is for the internals to be completely hidden from you; the value of an iterator may not necessarily be greater than that of the next element.

In that case you'll have one less iteration, and the condition else if (it == nameStorage.end()) will never be true. However, you can just simply add the body of that statement after the for loop, since once the loop terminates you've iterated to the end of the vector.

Answer 5

You haven't added any elements to nameStorage, therefore you can't iterate over the elements of that vector container. Since you are not checking the bounds on the vector iterators correctly in the for-loop, it's allowing you to enter your loop, and there you're dereferencing an iterator that isn't pointing to anything. By attempting to access memory you haven't been allocated, you've invoking undefined behavior which will most likely result in a segmentation fault or some other odd issue if you corrupt memory but fail to crash.