1

I have a class which is called Position:

class Position
{
    public:
    ... //Constructor, Destructor, ...

    private:

      signed int x_;
      signed int y_;
}

Then I have a vector which stores pointers of Positions:

std::vector<Position*> positions

How can I check if a Position is contained in the vector? For example, I have an object of a Position:

Position* p_ = new Position(0, 0);

And I want to check if the vector contains a Position with the same coordinates? Which operator do I have to overload?

Thanks, Barbara

今天春天
  • 941
  • 1
  • 13
  • 27
  • 6
    Why are you storing pointers to positions instead of the positions themselves? Seems like a recipe for bugs. – Borgleader Apr 13 '15 at 13:25
  • Yeah I am also quite unsure about this. I asked one of my teammates why he does this and he said it would be more efficient and faster and it should not be changed - EVER. – 今天春天 Apr 13 '15 at 13:28
  • 5
    Wow. I'm glad I'm not in this project. – Daniel Daranas Apr 13 '15 at 13:29
  • 3
    "it would be more efficient and faster and it should not be changed" .. Yeah right. Because indirection is soooo efficient. – Félix Cantournet Apr 13 '15 at 13:29
  • 3
    @user3347983 well you can stop listening to him then... Until he reads about cache optimization and locality of reference. – Quentin Apr 13 '15 at 13:29
  • Sorry, I am totally a beginner in C++ and pointers. Can you maybe explain to me why it is efficient / why not? – 今天春天 Apr 13 '15 at 13:32
  • Read this question: [Why should I use a pointer rather than the object itself?](http://stackoverflow.com/q/22146094/96780) and its answers. – Daniel Daranas Apr 13 '15 at 13:34
  • 2
    I also recommend you to read some of the books recommended in this question: [The Definitive C++ Book Guide and List](http://stackoverflow.com/q/388242/96780). – Daniel Daranas Apr 13 '15 at 13:36
  • 2
    @user3347983 It's a whole topic. The gist of it is that the CPU uses (several levels of) cache to load whole chunks of data from RAM. When you store and access elements contiguously, you *hit* the cache : the object you access is already loaded in the previous chunk. However, by using pointers (to non-contiguous objects) you force the CPU to hop around in memory, *missing* the cache and reloading it for each object. Upshot : it gets **slow**. – Quentin Apr 13 '15 at 13:37
  • Another reference is [this one](https://isocpp.org/wiki/faq/value-vs-ref-semantics#compos-vs-heap). Basically, you should learn and search for answers yourself, instead of believing the myths somebody else tells you. – Daniel Daranas Apr 13 '15 at 13:39
  • @DanielDaranas Since it is the second day I use C++ and our professor didn't make any words about this topic, you may understand that I will not verify every single explanation someone gives to me. – 今天春天 Apr 13 '15 at 13:47
  • 3
    @DanielDaranas I agree with you concerning the searching for the answers. However, too many beginners really don't know that what they've been told is dubious at best, and nonsense at worst. If the code compiles, then hey, it's "ok" to them until their program crashes or gives wrong results. – PaulMcKenzie Apr 13 '15 at 13:51
  • @Babs I gave recommendations according to the contents of your question. You may follow them or not, according to your free will. – Daniel Daranas Apr 13 '15 at 13:52
  • @PaulMcKenzie I'm just giving recommendations that I find useful for a beginner to actually _learn_ how to code in C++. I'm not actually in charge of his team. If I was, I'd make sure they learn. – Daniel Daranas Apr 13 '15 at 13:55

3 Answers3

5
auto it = find_if(positions.begin(), positions.end(), 
                 [=](position* p)
                 {
                    return p->x() == p_->x() && p->y() == p_->y();
                 });

if(it != positions.end())
{
       //object found
}

However, unless you have a real reason to store pointers in the vector (e.g. you're going to use polymorphism), storing objects directly is much simpler.

vector<position> v;
v.push_back(Position(1, 2));
...
Position p_(1, 4);
auto it = find_if(v.begin(), v.end(), 
                  [=](position p)
                  {
                      return p.x() == p_.x() && p.y() == p_.y();  
                  });
if(it != v.end())
{
   //position found
}

In the latter case it is possible to further simplify the code by overloading operator == for position.

bool operator == (position p1, position p2)
{
   return p1.x == p2.x && p1.y == p2.y; //assuming operator == is declared friend
}

Then you can

auto it = find(v.begin(), v.end(), p_);
Armen Tsirunyan
  • 130,161
  • 59
  • 324
  • 434
0

Add this to class Position

Public:
bool isSamePosition(position * p){
    return p->x == this->x && p->y ==this->y;
}

Then compare with all in the vector

bool unique = true;
for (int i = 0; i < positions.length(); ++i){
    if (new_position->isSamePosition(positions[i])
        unique = false;
}
if (unique==true)
 //do something like push_back vector
;
Armen Tsirunyan
  • 130,161
  • 59
  • 324
  • 434
jteez
  • 13
  • 5
0

And I want to check if the vector contains a Position with the same coordinates? Which operator do I have to overload?

If you had a vector of positions (instead of vector of pointers to positions) the operator you'd have to overload would be:

bool Position::operator==(const Position& p);

With this code, you could write (assuming you are using std::vector<Position> positions;, and not std::vector<Position*> positions;):

using std::find; using std::begin; using std::end;
const Position p{}; // element that is sought after
bool exists = (end(positions) != find(begin(positions), end(positions), p));

[comment:] Yeah I am also quite unsure about this. I asked one of my teammates why he does this [i.e. store by pointers] and he said it would be more efficient and faster and it should not be changed - EVER.

It is probably not more efficient, nor faster than storing by values. If you are not in a position to change the vector though, you will have to add the operator declared above, and also a predicate that compares a Position instance to the values in a Position pointer, and using that with std::find:

    const Position p{}; // element that is sought after
    auto matches_position = [&p](Position const* const x)
    {
        return x != nullptr // can positions in the vector be null?
            && p == *x;
    };

    bool exists = (end(positions) != find(begin(positions), end(positions),
        matches_position));

== Coping strategy ==

I would go for the first version (no pointers in the vector), by doing the following:

  1. create a new (minimalistic) project, that fills two separate vectors, with a bunch of randomized positions (fixed number of positions, between 2000 and 10000 instances or so); the vectors should contain positions by pointer and by value respectively, with the same values in each position (a position should be in both vectors, at the same index)

  2. perform the search for the same values in both vectors.

  3. repeat the searches multiple times (to average and minimize timing errors)

  4. take results to your colleague(s).

There are two outcomes from this: either your colleague is right (which seems pretty unlikely, but hey! who knows?) or he is wrong, and his code that "should never be changed" - well ... it should be changed.

utnapistim
  • 26,809
  • 3
  • 46
  • 82