[superhead91]

I have a function that creates a pointer to a class, inserts some information into that class, and then stores a unique id and the pointer into a map. I haven't finished my code, but I have a suspicion that every time I call the function, it's going to cause the pointer from the previous class to point to the current class. Basically it will only create one pointer and then keep switching what it points to. Can anyone with more experience tell me if my function will for sure do this? I only ask now because it's a large program, and this is a bug I want to catch early.

Code:

int Code_Processor::New_Prize(string id, string description, int points, int quantity){

    Prize *p;
    map <string, Prize *>::iterator pit;

    p = new Prize;

    if(points <= 0){ return -1; }
    if(quantity <= 0){ return -1; }

    p->id = id;
    p->description = description;
    p->points = points;
    p->quantity = quantity;

    for(pit = Prizes.begin(); pit != Prizes.end(); pit++){
        if(pit->second->id == id){ return -1; }
        else if(pit++ == Prizes.end()){
            Prizes.insert(make_pair(id, p));
        }
    }

    return 0;
}

 

[Kirmie]

Judging by "Prize *p;" and "p = new Prize;" I think you will be fine. You create a new pointer every time you run the function and you point to a new object every time you start the function. I haven't worked in C++ in a bit but I don't see why this would have the pointers all point the same object.

The problem you are worried about would happen if you passed in the Prize object and set that Prize object to be a new Prize I would think. There you could possibly run into the new Prize overwriting the old one in the same location so the pointers all point to the same spot in the memory.

 

[superhead91]

Hmmm... well what I'm thinking is that since the pointer is getting the same name every time, the program will only see one pointer. I need each Prize to have its own unique pointer. Maybe not though because each one is going to be put into a map with its own unique id, so maybe that unique id will allow the program to differentiate between the pointers, even though they have the same name. The map the prizes are going in is declared like this:
map <string, Prize *> Prizes

 

[Twinnuke]

map <string, Prize *>::iterator pit;

This line intrigues me.

The rest I understand but I have no idea what you are doing here.

 

[Kirmie]

The name of the pointer is irrelevant outside of the function and it is only relevant inside the function for each call of the function. If it worked any other way my life would be a living hell and threading the same function would be impossible.

You could just copy and paste most of the code to make a real simple test and put the matter to rest. Create the map. Then call the function that adds to the map twice in a way where data in one object would be different from data in the other. Then just output the data in a format where you can compare it. Small scale tests like this help.

Edit:
Quote:

Originally Posted by Twinnuke map <string, Prize *>::iterator pit; This line intrigues me. The rest I understand but I have no idea what you are doing here.

He is creating an iterator named "pit" for a map object that maps a string to a Prize pointer. Its a way of iterating through enumerable objects. Here is some documentation.

Edit 2: On a side not you could check to see if the ID exists in Prizes or the other bail out situations before even creating the pointer for a new object and just call Prizes.insert at the end of the function. This should give a slight speed up because checking those situations is always required but creating the new Prize object and pointer is not if those situations are met. Basically this...

Code:

int Code_Processor::New_Prize(string id, string description, int points, int quantity){

    Prize *p;
    map <string, Prize *>::iterator pit;

if(points <= 0){ return -1; }
    if(quantity <= 0){ return -1; }

for(pit = Prizes.begin(); pit != Prizes.end(); pit++){
        if(pit->second->id == id){ return -1; }
    }

    p = new Prize;

    p->id = id;
    p->description = description;
    p->points = points;
    p->quantity = quantity;

Prizes.insert(make_pair(id, p));

    return 0;
}

 

[superhead91]

Quote:

Originally Posted by Twinnuke map <string, Prize *>::iterator pit; This line intrigues me. The rest I understand but I have no idea what you are doing here.

Lol... yeah. Iterators are one of those ugly C++ functions. Do you know what a map is? This function is only part of my code. Outside the function a map is created, and an iterator is required to traverse the map.

And kirmie, thanks for the help. The reason I asked about this now is because I don't wanna have to try and find this after I write my whole program. I guess I need to figure out how to output a map to see the contents.

Edit:

Ninja'd on the iterator explanation... lol

 

[Kirmie]

Quote:

Originally Posted by superhead91 Lol... yeah. Iterators are one of those ugly C++ functions. Do you know what a map is? This function is only part of my code. Outside the function a map is created, and an iterator is required to traverse the map. And kirmie, thanks for the help. The reason I asked about this now is because I don't wanna have to try and find this after I write my whole program. I guess I need to figure out how to output a map to see the contents. Edit: Ninja'd on the iterator explanation... lol

Super ninja! I did it a second time. Guess I should have looked to see if a post popped in while editing. The second thing I added isn't too important most of the time but just a good practice to take.

 

[superhead91]

Quote:

Originally Posted by Kirmie Super ninja! I did it a second time. Guess I should have looked to see if a post popped in while editing. The second thing I added isn't too important most of the time but just a good practice to take.

Yeah. I understand. Creating a pointer I'm not going to end up using won't hurt me in a small program, but in anything large scale you've got memory leaks on your hands.

 

[lordikon]

Quote:

Originally Posted by superhead91 I have a function that creates a pointer to a class, inserts some information into that class, and then stores a unique id and the pointer into a map. I haven't finished my code, but I have a suspicion that every time I call the function, it's going to cause the pointer from the previous class to point to the current class. Basically it will only create one pointer and then keep switching what it points to. Can anyone with more experience tell me if my function will for sure do this? I only ask now because it's a large program, and this is a bug I want to catch early. Code: int Code_Processor::New_Prize(string id, string description, int points, int quantity){ Prize *p; map <string, Prize *>::iterator pit; p = new Prize; if(points <= 0){ return -1; } if(quantity <= 0){ return -1; } p->id = id; p->description = description; p->points = points; p->quantity = quantity; for(pit = Prizes.begin(); pit != Prizes.end(); pit++){ if(pit->second->id == id){ return -1; } else if(pit++ == Prizes.end()){ Prizes.insert(make_pair(id, p)); } } return 0; }

There is no need for the 'new' in this case. You can create 'Prize' without the new keyword, and because it remains in the map it will remain in scope, then your map is no longer a map of strings to pointers, but just a map of strings to prizes. Also, if you 'return -1' in your example above you end up leaking memory because of your use of 'new', but if you don't use 'new' you're fine. Also, as in the other thread, passing in your strings by reference will prevent the strings from being copied every time New_Prize() is called.

Code:

int Code_Processor::New_Prize(const std::string& id, const std::string& description, int points, int quantity)
{
    if(points <= 0){ return -1; }
    if(quantity <= 0){ return -1; }

    Prize p;
    p->id = id;
    p->description = description;
    p->points = points;
    p->quantity = quantity;

    // If a 'Prize' by 'id' doesn't exist, create it
    if ( Prizes.find(id) == Prizes.end() )
   {
         Prizes.insert(make_pair(id, p));
   }

    return 0;
}

Unrelated, I'm curious why the function returns an int instead of a bool.

You could also create a constructor in the class/struct 'Prize' to cleanup your assignments to it:

Code:

class Prize
{
     Prize(const std::string& id, const std::string& description, int points, int quantity) :
         m_id(id)
        , m_desc(description)
        , m_points(points)
        , m_quantity(quantity)
     {
     }
};

int Code_Processor::New_Prize(const std::string& id, const std::string& description, int points, int quantity)
{
    if(points <= 0){ return -1; }
    if(quantity <= 0){ return -1; }

    // This ends up looking cleaner and doesn't allow somebody
    // to make a 'Prize' without setting all required values.
    Prize p(id, description, points, quantity);

    // If a 'Prize' by 'id' doesn't exist, create it
    if ( Prizes.find(id) == Prizes.end() )
   {
         Prizes.insert(make_pair(id, p));
   }

    return 0;
}

 

[NorCa]

All i can add is remember to match every new with a delete just to be safe

 

[superhead91]

@lordikon The reason I the function is laid out the way it is is because this is an assignment for school, and the prototypes for the functions are all in a header file that the teacher has given us, so I can't really change them.

 

[lordikon]

Quote:

Originally Posted by superhead91 @lordikon The reason I the function is laid out the way it is is because this is an assignment for school, and the prototypes for the functions are all in a header file that the teacher has given us, so I can't really change them.

That's understandable. You might let your teacher know that passing strings by value causes a copy.

To be fair, he/she might not be using const references to strings because he's trying to keep the syntax a little bit more simple. Throwing 'const', ampersands and asterisks into the mix can be confusing for people newer to C++.