C++ question regarding inserting elements into vectors

Question

I am working through the examples in Accelerated C++. One of the problems asks the reader to copy records for students who pass exams into the beginning of a vector called students. fgrade is a function defined elsewhere which returns failing students. Then one has to use the resize function to remove the extra elements from students so it only contains those who pass. I have tried this code but it does not work. Could anyone tell me if the fault lies in the code below?

#include "stdafx.h" 
#include <vector>
#include "Student_info.h"
#include "grade.h"

using std::vector;

// second try: correct but potentially slow
vector<Student_info> extract_fails(vector<Student_info>& students)
{
    vector<Student_info> fail;
#ifdef _MSC_VER
    std::vector<Student_info>::size_type i = 0;
    std::vector<Student_info>::size_type count = 0;
#else
    vector<Student_info>::size_type i = 0;
    vector<Student_info>::size_type count = 0;
#endif

    while (i != students.size()) { 
        if (!fgrade(students[i])) { 
            students.insert(students.begin(), students[i++]);
            count++;
        }
        i++; 
    }
    students.resize(count); 
    return students;
}

You have i++ twice in your loop, therefore your code will skip all odd students. If students.size() is not even, the loop will go forever. — Artium, Jan 26 '11 at 10:24
in this state, you take a student in the vector and if the condition match, you insert the _same_ student in the vector. So you have it twice in the vector ... Maybe you need to use the 'fail' vector ... — PW., Jan 26 '11 at 10:27
Is it really a good idea to modify the vector _as you iterate over it_? Especially since you're using indexing to get through each student, duplicating the passing student into the beginning of the vector, you're relying on that duplicated `i++` in there to save your bacon. If it were me I would just use a second vector. — sarnold, Jan 26 '11 at 10:28

score 1 · Answer 1 · edited May 23 '17 at 12:18

1

You can use remove_if from std::algorithm, but the functor should return the people that haven't pass (f_not_grade) instead the people that has pass:

std::remove_if(students.begin(), students.end(), f_not_grade)

Or you can look the way to negate a functor here or here to use the f_grade function without modifications and the remove_if.

Most of the common operations with containers are implemented in the STL, so use the power of the language!. Taking a while to look for this kind of functions makes us to code less and better.

Edited to delete an incorrect "()".

edited May 23 '17 at 12:18

Community

1
1

answered Jan 26 '11 at 10:38

Jon Ander Ortiz Durántez

2,542
1
20
29

He will still have to resize after that, but that should not be a problem, one can just use the iterator returned by `remove_if` to calculate the length of the new sequence. This is more elegant than my solution. – Björn Pollex Jan 26 '11 at 10:44
Sorry, I was imprecise. fgrade returns a boolean indicating whether the student failed. I have tried using the above with fgrade() as the final parameter but this is failing. Likewise with fgrade == false; I know this final comment will seem stupid but I'm really stuck here. The book Accelerated C++ is good but the examples really hard for some of us. – Steblo Jan 26 '11 at 11:24
I think the authors want us to use insert() and resize(). I have rewritten as follows. I have put the students into a vector called pass. It runs but does not resize the vector to eliminate all students who have failed - they would be at the end. while (i !=students.size()) { if (!fgrade(students[i])) { pass.push_back(students[i]); count++; } else ++i; } students.insert(students.begin(),pass.begin(), pass.end()); students.resize(count); return students; } – Steblo Jan 26 '11 at 12:06
Hi Steve, the "()" after the f_not_grade in my comment is not correct. The correct line would be std::remove_if(students.begin(), students.end(), fgrade); sorry for the inconveniences. – Jon Ander Ortiz Durántez Jan 26 '11 at 12:48

score 0 · Accepted Answer · edited May 23 '17 at 10:33

You increment i twice in your loop.

A cool way to do it would be to use a custom predicate for std::sort:

bool CompareStudentsByFailure(const Student_info & left, 
                              const Student_info & right) {
   return fgrade(left) > fgrade(right);
}

And then use it like this:

std::sort(students.begin(), students.end(), CompareStudentsByFailure);
students.resize(std::distance(students.begin(), 
                std::find_if(students.rbegin(), 
                             students.rend(), fpgrade).base()));

However, Jon`s answer is a bit simpler.

score 0 · Answer 3 · answered Jan 26 '11 at 10:30

0

As Space_C0wb0y and Artium pointed out, you incremented i twice. Also, you declare the vector fail, but never use it (unless you're doing something with it later). In addition, the macro in the function seems like overkill--it basically says the same thing twice (minus the "std::") and you might as well just use ints--easier for the next person to read and understand.

answered Jan 26 '11 at 10:30

Gemini14

6,246
4
28
32

Thank you very much to all who helped me. You have been most helpful. – Steblo Jan 28 '11 at 09:41

C++ question regarding inserting elements into vectors

3 Answers3