Thread: using maps<>

please post the errors.

the problem is that you're trying to iterate over a list<int> with map<int, int> iterators. that will never work. they are not compatible types.

your logic is also flawed in that every time you encounter a number the same number in the list, you're resetting the count in the map to zero.

you're still resetting the count of each number to zero. you should change line 53 to something like this:

Code:

if (freq.find(*iter1) == freq.end()) freq[*iter1] = 0;

this will only set it to zero if it didn't find it in the map.

hey, sorry I've been away for a little bit. I'm not sure exactly where I'm resetting the count of each number.

Code:

 //p.345 ex1
//generates N numbers to a list<>
//finds num of occurances of values
//place in map<> where k=number/v=frequency

#include <iostream>
#include <cstdlib>
#include <ctime>
#include <list>
#include <map>

#define RND_RANGE 10000
#define TIMES 100

using namespace std;

int main(void){

    srand(time(NULL));

    //generate 10,000 numbers between 0 and 9,999
    list<int> the_list;
    list<int>::iterator iter;

    //place in list<int> container
    for(int i = 0; i < TIMES; i++){
        int j = (rand() % RND_RANGE);
        the_list.push_back(j);
    }

    //iterate over values and display them
    int x = 0;
    for(iter = the_list.begin(); iter !=the_list.end(); iter++){
        cout << "item: " << x << " = " << *iter << "..." << endl;
        if(x % 10 == 0){
            cout << "Press [ENTER] for next ten...";
            cin.get();
        }
        x++;
    }

    //create a map container to hold freq of each number
    map<int, int> freq;
    list<int>::iterator iter1, iter2;
    
    //iterate over each element in the list
    for(iter1 = the_list.begin(); iter1 !=the_list.end(); ++iter1){
        //create a key/value pair where k=num/v=freq, v=0
        freq[*iter1] = 0; 

        //iterate the list again while at current element to see if it repeates
        for(iter2 = the_list.begin(); iter2 !=the_list.end(); ++iter2){
            //if it occurs again increase the v count by 1
            if( *iter2 == *iter1){ 
                freq[*iter1]+=1;
            }
        }
    }
    
    //iterate over map to see key/value pairs
    map<int,int>::iterator map_itr;
    cout << "Number\tFrequency" << endl;
       for(map_itr = freq.begin(); map_itr !=freq.end(); ++map_itr){
            cout << (*map_itr).first << " appears " << map_itr->second << " times" << endl;
        }

    
    return 0;
}

In the above code the first loop creates a key value pair for the number in the list and assigns it to be zero. ...the second loop iterates from the beginning and increments the value in the map if that number is found again. So with the way i have it each number is guaranteed to say it appears at least once. I'm not sure what I'm missing so please spell it out if need be.

also I understand the usage of the method find() but the second loop accomplishes that.

I redid the code a little to bring it up to modern C++ (including C++11):

Code:

template<typename C, typename Elem, typename... T> void push_back(C& c, const Elem& value, const T&... values)
{
	c.push_back(value);
	push_back(c, values...);
}

template<typename C> void push_back(C&) {}

int main()
{
	const int RndRange = 10000;
	const int Time = 10;

	std::srand(std::time(nullptr));

	//generate 10,000 numbers between 0 and 9,999
	std::list<int> the_list;
	push_back(the_list, 1, 2, 2, 3, 4, 2, 3, 3, 5, 1, 0);

	//   //place in list<int> container
	//   for (int i = 0; i < Time; i++)
	//       the_list.push_back((rand() % RndRange));

	//iterate over values and display them
	{
		int item_c = 0;
		for (const auto & item : the_list)
			std::cout << "item: " << ++item_c << " = " << item << "..." << std::endl;
	}

	//create a map container to hold freq of each number
	std::map<int, int> freq;

	//iterate over each element in the list
	for (const auto & item : the_list)
		freq[item]++;

	//iterate over map to see key/value pairs
	std::cout << "Number\tFrequency" << std::endl;
	for (const auto & item : freq)
		std::cout << item.first << " appears " << item.second << " times" << std::endl;

	return 0;
}

Thing to take home:
- Don't use #define. Use const T, where T is a type.
- Don't use NULL. Use nullptr if you can (C++11).
- No need for global variables, even if they're constants. Limit scope.
- No need to declare iterators before the loops. Declare them inside the loops so you don't have to poison the function scope.
- Use range based for if you can (C++11).
- Use auto to deduce the types for iterators automatically, if possible (C++11).
- Use the find method of map to see if an element exists, and if not, insert it (initialize it to 0). If you want to be more efficient, you can initialize it to one and skip incrementing it.
- No need to use += for increments of 1. Use the ++ operator!
- operator [] constructs a new element with a default-constructed value with the specified key if it doesn't exist. That means that if you try to access some key k, and it doesn't exist, it will be created with value 0. Therefore, you really don't have to initialize anything. You can just increment away directly! Efficient and easy.
- The global push_back function isn't really necessary to understand or know. It's a helper function to help inserting values into the list (instead of randomizing it for easier testing). Visual Studio's support for initializer lists is poor, which is why I had to do it. In gcc or clang, you'd use initializer lists to initialize it (unless you do your loop to randomize the values inserted, of course). The global push_back function uses variadic templates. Variadic templates and initializer lists are both C++11 features.

Originally Posted by hex_dump

Not bad...i like what you did on lines 35 and 36...I'm assuming this is what checks to see if a value occurs and then increments its value? I copied and pasted it but Vim made a mess of it so it didn't compile.

The [] operator inserts a default-constructed value (for integers, it's 0) if it doesn't exist. So it creates it if it doesn't exist and initialized it to 0, then returns it which is incremented. If it exists, then the existing element is returned.
As for the compile errors, make sure your compiler supports C++11 and that your compiler is set to follow it (gcc, for example, needs the -std=c++11 switch). If it doesn't support C++11, this code won't compile.

-the push_back method only works on sequence containers...not associative ones so this is guaranteed to fail on a map or set right?

I don't see push_back on maps and sets

-so considering the aforementioned, and the fact that we're still invoking the push_back method, what's the point of putting in a function?

That was just a small hack I used for easily inserting values when testing. Msvc's standard library doesn't support initializer lists. If I didn't use it, I'd have to do

the_list.push_back(1);
the_list.push_back(2);
the_list.push_back(2);
//...

-again considering that we created a template function to do that work, why seed the SRNG? since we never actually used it.

Again, that was just for my testing. If you intend to randomize values for inserting, you definitely need it.

-I'm still new to C++ but any reason for the braces on lines 25 thru 29?

To stop the item_c variable from poisoning the function scope. I only intend to use it within the loop, but there's no way to declare variables inside a range-based for loop (it can only be done inside, at which point they'll be destroyed on every iteration, or outside). You can't declare two different variables with two different types inside normal for loops, either, hence the extra scope.

-and lastly something I've been meaning to find out...what is with the convention of using the prefix notation on loops in C++?

For iterators, prefix is faster because the postfix operator must construct a temporary of itself and return it (remember that the value returned by postfix is the value before it's incremented?). Prefix does not such thing, so other than that, it's a matter of style, pretty much.

Originally Posted by whiteflags

Does this do what I think it does? It is NOT NOT NOT a good idea to fill a list by using a function with 10000 arguments.

If you mean that it recursively calls the function once for each argument? Then yes.
But then again, would you type out 10 000 arguments in the call to that function? I sure as heck wouldn't. And that's really the only use of it, anyway.

But then again, that got me thinking. Why use variadic templates when all arguments are of the same type?
If that's the case, then the follow would suffice:

Code:

template<typename C, typename Elem> void push_back(C& c, std::initializer_list<Elem> values)
{
	for (const auto & elem : values)
		c.push_back(elem);
}

auto args = { 1, 2, 2, 3, 4, 2, 3, 3, 5, 1, 0 };
push_back(the_list, args);

(Again, C++11 features.)

EDIT2:
But then again, it might be a good idea to use r-values in the function since I would be passing temporaries in most of the cases (that's really what I designed it for), so a better version might be:

Code:

template<typename C, typename Elem, typename... T> void push_back(C& c, const Elem& value, const T&&... values)
{
	c.push_back(value);
	push_back(c, std::move(values)...);
}

template<typename C> void push_back(C& c) {}

It was for testing purposes.
It is easier to insert 10 specific values and check that it holds or insert 100 random values and check it holds.
But yeah, the reason for having it remain the code I showed off is ... pretty much because I wanted to show it off. And give a little taste of the power in C++.