Storing dynamic array in vector

Question

I have to display a histogram of a student's grades. I've stored the grades in a dyn. array, but my goal is to store them in a vector. What's the right way of going about this? Hope this makes sense.

Edit:

My attempt at using a vector

void displayHistogram(int minGrade, vector<int> ptrV) {

cout << endl;
for (int i = 0; i <= minGrade; i++) {
    if (ptrV[i] != 0) {
        cout << "Number of " << i << "'s: " << ptrV[i] << endl;
    }
}

}

void histogram() {

int minGrade = 0, grade;
const int grade_max = 100;
vector<int> ptrV(grade_max, 0);

cout << "Enter the student's grades (-1 to stop entering): \n";
do {
    cin >> grade;
    if (grade > minGrade) {
        minGrade = grade;
    }
    if (grade >= 0) {
        ptrV.push_back(grade);
    }
} while (grade != -1);

displayHistogram(minGrade, ptrV);

}

Answer 1

Your basic mistake is that you try to force the vector as if it was a raw array. It does stuff for you, let it. It knows it's size, for instance. You don't need

void displayHistogram(int minGrade, vector<int> ptrV) {

    cout << endl;
    for (int i = 0; i <= minGrade; i++) {

Instead, you can use vector::size:

void displayHistogram(vector<int> ptrV) {

    cout << endl;
    for (size_t i=0; i<ptrV.size(); i++) {

(Even better: void displayHistogram(const vector<int>& ptrV) to signify that ptrV is not changed here and to avoid copying it every time you call the function by using a reference.)

(If you wouldn't use the i as it is the grade and if you have a newer compiler, I'd recommend a for each loop instead. Those are usually the way to go, just happens that you have one of the rarer cases in which it isn't.)

Likewise, you first set the size of your vector and then increase it, which to me means that you do not trust it:

 vector<int> ptrV(grade_max, 0);

At this point, you have a vector with a hundred entries that are all zero. You don't need to resize it later if a hundred entries is all you need. vector::push_back resizes it. But note that setting it to a size of 100 means that [100] is not a valid position, the last one is [99], as we start to count at zero. You'd need to set it to a size of 101 to have both zero and hundred as valid addresses.

I'd change your code to:

const int grade_max = 100;
vector<int> ptrV(grade_max+1, 0); //changed it to +1 here as prtV[100] should be legal

cout << "Enter the student's grades (-1 to stop entering): \n";
while (true)
{
    int grade; // put stuff in the smallest scope possible
    cin >> grade;
    if(grade == -1) break; // doing that here means we don't have to think about it anymore - the do while does it at last, I do it at first, handling all the special cases at the start and then assume I have the regular case.
    if(grade < 0 or grade > grade_max) continue; // continue jumps to the top of the most inner loop. Note that I make sure to catch illegal but possible input.

    ptrV[grade] += 1; // personal preference, I use ++ only to iterate
}

displayHistogram(ptrV);

I rewrote the structure, using a while(true), I think the way I did it is more intuitive, but there will be people who disagree with that and who would also write things like

if(grade == -1)
{
    break;
}

and there are some good arguments for that, mostly a good practice routine, always doing the braces to avoid errors. However, I prefer a one liner to reduce verbosity.

One improvement would also be to tell the user about bad input:

if(grade < 0 or grade > grade_max)
{
    cout << "Input not in valid range. Please choose number within 0 to " << grade_max << endl;
    continue;
}

Now, another big thing to do here would be to leave the procedural part, by the way. Go for a class GradeHistogram which has all those functions as a part of it, being called like

GradeHistogram histogram;
histogram.take_input();
histogram.display();

but that is for when you get your code working.

(My answer became more of a review as found on CodeReview, but I think that this is what you need rather than small fixes. This is something I can only recommend you, by the way, putting your code on CodeReview once it works.)

Answer 2

If what you want to show is a histogram, then the easiest thing to do would be to use a std::map from grade to count of grade.

Something like this:

#include <iostream>
#include <map>
int main() {
    std::cout << "Enter the student's grades (-1 to stop entering): \n";
    std::map<int, int> grades_map;
    int input_grade = -1;
    do {
        cin >> input_grade;
        if (input_grade > -1) {
            grades_map[input_grade]++;
        }
    } while (input_grade != -1);

    // Print histogram
    for (const auto& [grade, count] : grades_map) {
        std::cout << "Students with grade << grade << ": ";
        for (int i = 0; i < count; ++i) {
            std::cout << '*';
        }
        std::cout << '\n';
    }
}

Answer 3

but my goal is to store them in a vector.

The issue seems to be that you've already sized the vector to hold grade_max entries. However when filling the vector, you are using push_back. By using push_back, you are adding more entries to the end of the vector, which is not what you want to do.

The solution is either

1. Change this vector<int> ptrV(grade_max, 0); to this vector<int> ptrV; and leave the calls to push_back alone, or
2. Keep vector<int> ptrV(grade_max, 0); but instead merely use ptrV[i] = grade;