Count the frequency of items in an array

Question

The question is:

Write a program that prompts the user to input ten values between 80 and 85 and stores them in an array. Your program must be able to count the frequency of each value appears in the array.

Is there any more efficient ways to present the count for frequency number between 80 to 85? Below is the sample code which had been done in the simplest ways to count the frequency for each number.

The part which needs more efficient ways to present:

 for(int i = 0; i < inputSize; i++) {
        if(inputValue[i] == 80) {
            tempCount80++;
        } else if(inputValue[i] == 81) {
            tempCount81++;
        } else if(inputValue[i] == 82) {
            tempCount82++;
        } else if(inputValue[i] == 83) {
            tempCount83++;
        } else if(inputValue[i] == 84) {
            tempCount84++;
        } else if(inputValue[i] == 85) {
            tempCount85++;
        } else {
            cout << "Error Accurs." << endl;
        }
    }

    cout << 80 << "      " << tempCount80 << endl;
    cout << 81 << "      " << tempCount81 << endl;
    cout << 82 << "      " << tempCount82 << endl;
    cout << 83 << "      " << tempCount83 << endl;
    cout << 84 << "      " << tempCount84 << endl;
    cout << 85 << "      " << tempCount85 << endl;

The original code is: -

#include <iostream>
using namespace std;

int main() {
    const int inputSize = 10;
    int inputValue[inputSize];
    int tempCount80 = 0;
    int tempCount81 = 0;
    int tempCount82 = 0;
    int tempCount83 = 0;
    int tempCount84 = 0;
    int tempCount85 = 0;

    for(int i = 0; i < inputSize; i++) {
        int tempValue = 0;
        cout << "Please enter a number between 80 and 85: ";
        cin >> tempValue;

        if(tempValue > 79 && tempValue < 86) {
            inputValue[i] = tempValue;
        } else {
            cout << "The number must be between 80 and 85" << endl;
            do {
                i--;
                break;
            } while(i > 0);
        }
    }

    for(int i = 0; i < inputSize; i++) {
        if(inputValue[i] == 80) {
            tempCount80++;
        } else if(inputValue[i] == 81) {
            tempCount81++;
        } else if(inputValue[i] == 82) {
            tempCount82++;
        } else if(inputValue[i] == 83) {
            tempCount83++;
        } else if(inputValue[i] == 84) {
            tempCount84++;
        } else if(inputValue[i] == 85) {
            tempCount85++;
        } else {
            cout << "Error Accurs." << endl;
        }
    }

    cout << 80 << "      " << tempCount80 << endl;
    cout << 81 << "      " << tempCount81 << endl;
    cout << 82 << "      " << tempCount82 << endl;
    cout << 83 << "      " << tempCount83 << endl;
    cout << 84 << "      " << tempCount84 << endl;
    cout << 85 << "      " << tempCount85 << endl;

    return 0;
}

The output is as expected, but need to find more efficient ways to solve the question.

Answer 1

Answer to the Performance Issue
Anytime there are a list of variables named NAMEi where i is an integer, there is a strong chance that a container such as std::array or std::vector should be used. Sometimes using a table rather than multiple if statements can improve performance.

Indexing into an array will prevent the repetitive code that is currently in the solution:

#include <array>

int main()
{
const int inputSize = 10;
const int frequencyCount = 6;
std::array<int, inputSize> inputValues;
std::array<int, frequencyCount> freqs;

    ...
}

The array freqs will contain the frequency of the occurrence, there are a couple of ways to index into the array freqs. One would be to subtract 80 from the input value.

This will reduce the multiple if statements into a simple increment of an item in an array. It will also reduce the amount of code necessary to print the frequencies.

When performance is an issue, prefer "\n" over std::endl. The use of std::endl flushes the output which may mean there is a system call for each use. A system call can add a great deal of time.

Remove the do/while loop in the error checking.

Use the Container Classes Provided by C++
The code currently appears to be C rather than C++. It is using the old style C arrays, C++ supplies an array container class as part of the standard library. Using the array container class would allow you to use iterators instead of indexes, at least for printing. Here is a second reference.

Avoid Using Namespace STD
If you are coding professionally you probably should get out of the habit of using the "using namespace std;" statement. The code will more clearly define where cout and other functions are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The function cout you may override within your own classes. This stack overflow question discusses this in more detail.

DRY Code
There is a programming principle called the Don't Repeat Yourself Principle sometimes referred to as DRY code. If you find yourself repeating the same code mutiple times it is better to encapsulate it in a function. If it is possible to loop through the code that can reduce repetition as well.

Complexity
The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principe that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

There are at least 3 possible functions in main().
- Get the user input
- Get process the frequencies
- Print the frequencies

Magic Numbers
There are Magic Numbers in the main() function (79 and 86), it might be better to create symbolic constants for them to make the code more readble and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintainence easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

Answer 2

using namespace std; is a bad habit you should avoid.

---

Input Handling:

for(int i = 0; i < inputSize; i++) {
    int tempValue = 0;
    cout << "Please enter a number between 80 and 85: ";
    cin >> tempValue;

    if(tempValue > 79 && tempValue < 86) {
        inputValue[i] = tempValue;
    } else {
        cout << "The number must be between 80 and 85" << endl;
        do {
            i--;
            break;
        } while(i > 0);
    }
}

Although we want exactly inputSize inputs, we may have to request the input multiple times from the user. Doing this by decrementing i like that is quite inventive, but it's probably neater to use another loop to repeat the input request until we get a valid input:

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

    int number = 0;

    while (true) {

        std::cout << "Please enter a number between 80 and 85: ";
        number = 0;
        std::cin >> number;

        if (number >= 80 && number <= 85) {
            break;
        }
        else {
            std::cout << "The number must be between 80 and 85.\n";
        }
    }

    inputValue[i] = number;
}

The inner loop could then be moved to a separate function, so the outer loop would simply look like:

for (int i = 0; i != inputSize; ++i) {
    inputValue[i] = getInput();
}

Note that we should add error handling code to ensure that the user input is valid (e.g. what if the user enters "abc", or a number too large to fit in an int?). This is quite awkward in C++, but ends up looking something like this:

#include <iostream>
#include <limits>

int getInput() {

    while (true) {

        std::cout << "Please enter a number between 80 and 85: ";

        int number = 0;
        std::cin >> number;

        std::cout << "\n";

        if (std::cin.eof()) {
            std::cout << "Unexpected end of file.\n";
            std::cin.clear();
            continue;
        }

        if (std::cin.bad() || std::cin.fail()) {
            std::cout << "Invalid input (error reading number).\n";
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            continue;
        }

        if (number < 80 || number > 85) {
            std::cout << "Invalid input (number out of range).\n";
            continue;
        }

        return number;
    }

    // unreachable
    return 0;
}

int main() {

    int number = getInput();

    std::cout << number << std::endl;
}

---

Frequency count:

Rather than individual variables, we can use another array here. This removes a lot of the repetition. Something like:

const int freqSize = 6;
int frequencies[freqSize] = { 0, 0, 0, 0, 0, 0 };

for (int i = 0; i != inputSize; ++i) {
    int binIndex = inputValue[i] - 80;

    assert(binIndex >= 0 && binIndex < freqSize); // need #include <cassert>
    frequencies[binIndex] += 1;
}

for (int i = 0; i != freqSize; ++i)
    std::cout << (i + 80) << "      " << frequencies[i] << "\n";

We should probably define our min (80) and max (85) values as constant variables somewhere, instead of using "magic numbers".

---

Note that in "real" C++ code, we would probably use a data structure such as std::map<int, int> to store the count of each input value, and avoid using C-style arrays completely.