Mean and Mode of vector array - How can I make a smaller improvement in the function

Question

Doing an exercise to find the mean and mode of a list of numbers input by a user. I have written the program and it works, but I'm wondering if my function 'calcMode' is too large for this program. I've just started looking into functions which is a first attempt. Would it be better to write smaller functions? and if so what parts can I split? Im pretty new to C++ and also looking if I can improve this code. Is there any changes I can make to make this run more efficient?

#include<iostream>
#include<vector>
#include<algorithm>
using namespace std;

int calcMean(vector<int> numberList) 
{

    int originNumber = numberList[0];
    int nextNumber;
    int count = 0;
    int highestCount = 0;
    int mean = 0;

    for (unsigned int i = 0; i <= numberList.size() - 1; i++) 
    {
            nextNumber = numberList[i];
            if (nextNumber == originNumber) 
                count++;
            else 
            {
                cout << "The Number " << originNumber << " appears " << count << " times." << endl;
                count = 1;
                originNumber = nextNumber;
            }
        }

    if (count > highestCount)
    {
        highestCount = count;
        mean = originNumber;
    }
    cout << "The Number " << originNumber << " appears " << count << " times." << endl;     
    return mean;
}

int main() 
{
    vector<int> v;
    int userNumber;
    cout << "Please type a list of numbers so we can arrange them and find the mean: "<<endl;

    while (cin >> userNumber)  v.push_back(userNumber);

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

    for (int x : v)    cout << x << " | ";

    cout << endl;
    cout<<calcMean(v)<<" is the mean"<<endl;
    return 0;
}

Answer 1

One thing to watch out for is copying vectors when you don't need to.

The function signature

int calcMode(vector<int> numberList)

means the numberList will get copied.

int calcMode(const & vector<int> numberList)

will avoid the copy. Scott Meyer's Effective C++ talks about this.

As an aside, calling is a numberList is misleading - it isn't a list.

There are a couple of points that are worth being aware of in the for loop:

for (unsigned int i = 0; i <= numberList.size()-1; i++)

First, this might calculate the size() every time. An optimiser might get rid of this for you, but some people will write

for (unsigned int i = 0, size=numberList.size(); i <= size-1; i++)

The size is found once this way, instead of potentially each time. They might even change the i++ to ++i. There used to a potential overhead here, since the post-increment might involve an extra temporary value

One question - are you *sure this gives the right answer? The comparison nextNumber == originNumber is looking at the first number to begin with. Try it with 1, 2, 2.

One final point. If this is general purpose, what happens if the list is empty?

Answer 2

Would it be better to write smaller functions?

• Yes, you can make do the same job using std::map<>; which could be a much appropriate way to count the repetition of the array elements.
• Secondly, it would be much safer to know, what is the size of the array. Therefore I suggest the following:

std::cout << "Enter the size of the array: " << std::endl; std::cin >> arraySize;

• In the calcMode(), you can easily const reference, so that array will not be copied to the function.

Here is the updated code with above mentioned manner which you can refer:

#include <iostream>
#include <algorithm>
#include <map>

int calcMode(const std::map<int,int>& Map)
{
    int currentRepetition = 0;
    int mode = 0;

    for(const auto& number: Map)
    {
        std::cout << "The Number " << number.first << " appears " << number.second << " times." << std::endl;

        if(currentRepetition < number.second )
        {
            mode = number.first;  // the number
            currentRepetition = number.second; // the repetition of the that number
        }
    }
    return mode;
}

int main()
{
    int arraySize;
    int userNumber;
    std::map<int,int> Map;

    std::cout << "Enter the size of the array: " << std::endl;
    std::cin >> arraySize;

    std::cout << "Please type a list of numbers so we can arrange them and find the mean: " << std::endl;
    while (arraySize--)
    {
        std::cin >> userNumber;
        Map[userNumber]++;
    }

    std::cout << calcMode(Map)<<" is the mode" << std::endl;
    return 0;
}

Update: After posting this answer, I have found that you have edited your function with mean instead of mode. I really didn't get it.

Regarding mean & mode: I recommend you to read more. Because in general, a data set can have multiple modes and only one mean.

Answer 3

I personally wouldn't split this code up in smaller blocks, only if i'd want to reuse some code in other methods. But just for this method it's more readable like this.

The order of excecution is aroun O(n) for calc which is quite oke if you ask me