Count the frequency

[garyfang]

hello guys, could u pls help me with my frequency function?
I was making a function to count the frequency of those elements inside the vector. But i think some bugs in my code, and i can't figure out where is it.
Pls help!

  void count(vector<int> arr){
     sort(arr.begin(), arr.end());
    int frequency = 1;

    for (int x =0; x < arr.size(); x ++){
        if(arr[x]==arr[x+1]){
            frequency ++;
        }
        else{
            cout << arr[x] << ": " << frequency << endl;
            frequency = 1;
        }
    }
}

[Kevin C]

That seems quite OK to me , except:

x < arr.size()

Which should be changed to:

x < arr.size() - 1

(otherwise, accessing arr[x+1] is out of array bounds)

[garyfang]

@Kevin C if we switch arr.size()-1, we will lose the last value right??
i meant that at the end if i have a list {1,1,1,1,1,2,2,2,2,2,2} i think we somehow will lose the value 2 right ?

[keskiverto]

Kind of yes. Then again, the arr[arr.size()] is definitely an out of range error.

You could use std::unique_copy and std::count. That should be safe.

[cire]

@Kevin C if we switch arr.size()-1, we will lose the last value right??

As written, yes. But since your code is already wrong... perhaps you should rethink your algorithm.

// http://ideone.com/okYq8p

void print_frequencies(std::vector<int> v)
{
    std::sort(v.begin(), v.end());

    if (!v.empty())
    {
        auto prev = v.front();
        std::size_t freq = 1;

        auto out = [&] { std::cout << prev << ": " << freq << '\n'; };

        for (std::size_t i = 1; i < v.size(); ++i)
        {
            if (prev == v[i])
                ++freq;
            else
            {
                out();
                prev = v[i];
                freq = 1;
            }
        }
        out();
    }
}

Note how we don't need to restrict our printing to the body of the for loop.

[Kevin C]

Oh,yes I for got about the last value. How's about this (I didn't test it):

  void count(vector<int> arr){
     sort(arr.begin(), arr.end());
    int frequency = 1;

    for (int x =0; x < arr.size(); x ++){
        if(x!=arr.size()-1 && arr[x]==arr[x+1]){
            frequency ++;
        }
        else{
            cout << arr[x] << ": " << frequency << endl;
            frequency = 1;
        }
    }
}

[YFGHNG]

This isn't an answer, but I'm building on certain things that

Kevin C

and

keskiverto

have said.

I assume you're familiar with a vector object having an automatic growth in size every time you insert a piece of data into a vector that spills over in .size(). You gave us an example of your vector having these elements: {1,1,1,1,1,2,2,2,2,2,2} with eleven(11) elements inside of it. Correct me if I'm wrong on the counts, but the point will stay the same.

Now let's walk through your for loop: let's say we're at the last step, where

x = 10
// therefore, your code does its stuff and x increments
// now the problem becomes arr[10]==arr[11] 

As you now may have noticed, what's supposed to be in the 12th slot of your vector "arr"?
arr[10] will refer to your last 2 in the example data.

As a side note, I probably wouldn't name a vector as "arr". IMO that's just horrible naming sense. Typically, you'd want to note what an object or a function is used for, like getInt(), or getNonNegativeInt() or other similar functions, with parameters inside like upperBound, limit, etc. Also not mentioning logical size vs physical size.

[garyfang]

I think all i need to change is that after the for loop i have cout the last element of the vector and it's frequency.
and that's pretty much about the program