1

My remove_if seems to be overwriting the elements that are not filtered out with values of filtered out elements. The purpose of these code is to allow user to filter and display only teacher from a certain category. (Not deleting any element) Here are some of the code

static string compare;
static string debug;

bool filter_Cat (Teacher &t) 
{ 
    return (t.getCat() != compare); 
}

void filterCat (vector<Teacher> &t)
{
   vector<Teacher>::iterator i;
   vector<Teacher>::iterator newedited = remove_if(t.begin(), t.end(), filter_Cat);
   for (i = t.begin(); i != newedited; ++i)
   {
     Teacher& te = *i;
     te.getName();
     cout << "\t";
     te.getCategory();
     cout << "\t";
     te.getLocation();
   }
 }

 void filterTutorCat(vector<Teacher> &t)
 {
    int choice;
    cout << "No\tCategory" << endl
         << "1\tEnglish" << endl
         << "2\tMath" << endl
         << "3\tScience" << endl
         << "Choose the category you wish to filter :";
    cin >> choice;
    getline(cin, debug);

    if(choice <= 3 && choice > 0)
    {
        if (choice == 1)
        {
          compare = "English";
          filterCat(t);
        }
        if (choice == 2)
        {
          compare = "Math";
          filterCat(t);
        }
        if (choice == 3)
        {
          compare = "Science";
          filterCat(t);
        }

    }
    else
    {
        cout << "Invalid Option" << endl;
    }
 }
delphi316
  • 201
  • 2
  • 4
  • 10
  • 1
    What's the input, the expected output and the actual output? – Benjamin Lindley Jan 21 '12 at 06:01
  • As you can see from the code, the user can only enter 1,2, or 3 for which category they wish to filter. For the expected output, it should only display teacher from the category the user has chosen. The actual output came out right if there is only one object with the category matching the "compare" but if there is 2 object with category matching the "compare", it starts to overwrite the objects in the vector – delphi316 Jan 21 '12 at 06:21

2 Answers2

2

remove_if shifts elements, for which the compare function returns false, from right to left; which in other words means, it overwrites the elements, for which compare returns true, with elements, for which compare returns false. The size of the vector doesn't change, however.

This reads,

Removes all elements satisfying specific criteria from the range [first, last). The first version removes all elements that are equal to value, the second version removes all elements for which predicate p returns true.

Removing is done by shifting the elements in the range in such a way that elements to be erased are overwritten. The elements between the old and the new ends of the range have unspecified values. Iterator to the new end of the range is returned. Relative order of the elements that remain is preserved.

So what you want to do should be expressed as:

void filterCat (vector<Teacher> &v)
{
   for (vector<Teacher>::iterator it = v.begin(); it != v.end() ; ++it)
   {
      if (!filter_Cat(*i))
      {
           std::cout << i->getName() <<"\t" << i->getCategory() << std::endl;
      }
   }
 }

It seems in your code, getName() prints the name which ideally it should not do, instead it should return name. So I would suggest you to change it to make it return name. And do the same for getCategory as well. Choose your name correctly. If it is getName(), you should get you name by returning it; if it is printName(), then it should print name.


Also, the code which you've written isn't good:

  • You should avoid global variables.
  • You should avoid if-else as much as possible. Learn better ways.
  • You should learn about function objects (or functor)
  • You should learn about const member function.
  • You should understand the difference between iterator and const_iterator, and their usage.
  • You should understand the difference between const reference, and non-const reference. And try using them appropriately.

So I would write your code as:

//this is functor, not a function
struct filter_cat
{
   std::string m_cat; //use member data, avoid global variable
   filter_cat(std::string const & cat) : m_cat(cat) {}
   bool operator()(Teacher const & t) const  //const member function
   { 
     return (t.getCat() != m_cat); //getCat should be const member function
   }
};

//pass vector by const reference
void filterCat (vector<Teacher> const & v, filter_cat filter)
{
   //use const_iterator here, instead of iterator 
   for (vector<Teacher>::const_iterator it = v.begin(); it != v.end() ; ++it)
   {
      if (!filter(*i))
      {
           //getName and getCategory should be const member function
           std::cout << i->getName() <<"\t" << i->getCategory() << std::endl;
      }
   }
}

void filterTutorCat(vector<Teacher> const &t)
{
    int choice;
    cout << "No\tCategory" << endl
         << "1\tEnglish" << endl
         << "2\tMath" << endl
         << "3\tScience" << endl
         << "Choose the category you wish to filter :";
    cin >> choice;
    getline(cin, debug);

    //avoid if-else as much as possible, learn better ways!
    std::string cats[] = {"English", "Math", "Science"};

    if(choice <= 3 && choice > 0)
    {
          filterCat(v, filter_cat(cats[choice-1]));
    }
    else
    {
        cout << "Invalid Option" << endl;
    }
}

As noted in the comments: getCat, getName and getCategory should be const member functions. In fact, if getCategory returns category, then getCat isn't even needed.

Solved my issue.

Community
  • 1
  • 1
Nawaz
  • 353,942
  • 115
  • 666
  • 851
  • 1
    +1 for explanation in simple lucid language. The no of elements in the container wont change unless they are explicitly `erase`d. – Alok Save Jan 21 '12 at 06:21
  • I understand your coding but is there a way to do this with algorithm? – delphi316 Jan 21 '12 at 06:40
  • @NewUserSeekingHelp: Even if there is a way, it would be complex enough to avoid it. There is no straightforward way to do that using ``. Also, you should focus more on overall code, and other problems in your code, as I suggested in the re-write of your code. – Nawaz Jan 21 '12 at 06:51
  • Thank you so much for pointing out my mistakes but the problem is my lecturer insisted on us demonstrating the use of stl algorithm – delphi316 Jan 21 '12 at 06:52
  • *difference between const reference, and non-const reference* is probably better said as *difference between reference to a const, and reference to a non-const*. References themselves are inherently contants. – Alok Save Jan 21 '12 at 07:06
  • @Als: Yup. That is better expressed. But since, as you said, *"References themselves are inherently contants"*, then the phrase *"const reference"* is increasingly becoming a short-form of what you said, as it would mean only that. – Nawaz Jan 21 '12 at 07:13
  • bool operator()(Teacher const & t) const { return (t.getCat() != m_cat); }My compiler states that the t.getCategory() has type qualifier that are not compatible with the member function } – delphi316 Jan 21 '12 at 07:20
  • @NewUserSeekingHelp: You have both functions in your code? `getCat` and `getCategory`? Also, post the code at `pastebin.com`, and give the link in your question. Post the exact compilation error as well. – Nawaz Jan 21 '12 at 07:39
  • 1
    @Nawaz Thanks for the help but I've solved the issue. After your correction, I feel that I've improved my coding ethic! Thank you very much once again.. – delphi316 Jan 21 '12 at 07:49
1

remove_if collects the values for which filter_Cat returns false at the start of the container. While it doesn't reduce the number of elements in the container it neither does make any guarantees about the values of the elements beyond the returned range. So you are loosing values when using remove_if.

Eelke
  • 20,897
  • 4
  • 50
  • 76
  • So what would be a better option for my case? – delphi316 Jan 21 '12 at 06:23
  • @NewUserSeekingHelp: A for loop with a test in the loop to decide whether or not you want to display it. – Benjamin Lindley Jan 21 '12 at 06:25
  • Any algorithm I can apply that is suitable for this scenario? – delphi316 Jan 21 '12 at 06:27
  • @NewUserSeekingHelp: You could use `remove_if`, but you'd want to do it on a copy of the vector, not the actual vector. It's a wildly inefficient way to accomplish your task, but it would do what you are wanting. You could modify your current code by simply removing the ampersand from the function parameter. i.e. `void filterCat (vector t)` -- Then you're taking the vector by value. A copy is made, and none of the modifications affect the original. – Benjamin Lindley Jan 21 '12 at 07:10