Jump to content
  • Advertisement
Sign in to follow this  
supercoder74

strange vector problem

This topic is 4845 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

ok, I have this function that iterates through a vector. here it is:
#include<algorithm>
void character::displayitems()
{
     
     
     if(i.getitems().empty())
     {
      cout<<"you are carrying nothing."<<endl;
      return;
     }
     
     cout<<"you have:"<<endl;
     std::for_each(i.getitems().begin(),i.getitems().end(),print);

}

i is the class inventory, which is just pretty much a vector of items. item just has the string name in it. getitems returns a copy of the vector. and this is the print function:
static void character::print(item i)
{
     cout<<"  ";
     cout<<i;
     cout<<endl;
    
}

This gives me the encountered a problem and needs to close bit in windows, anyone know why?

Share this post


Link to post
Share on other sites
Advertisement
Guest Anonymous Poster
Quote:
Original post by supercoder74
getitems returns a copy of the vector.

Most likely this is the problem. You call getitems() multiple times so in effect you are iterating through a vector with non-related iterators.

Either store the value returned by getitems() as a local variable or modify getitems() to return a reference (constant or no).

Share this post


Link to post
Share on other sites
Quote:
Original post by Anonymous Poster
Quote:
Original post by supercoder74
getitems returns a copy of the vector.

Most likely this is the problem. You call getitems() multiple times so in effect you are iterating through a vector with non-related iterators.

Either store the value returned by getitems() as a local variable or modify getitems() to return a reference (constant or no).


hmm. Thats what I thought too. I change it to be a reference, but it still generates the error.I also slightly fixed the print function,because I noticed there were two 'i' instances. What else could it be?
EDIT:
I also tried the local variable idea. That didnt work either.

Share this post


Link to post
Share on other sites
I agree with the Anonymous Poster, I think getItems might be the a problem. Can you post your code for getItems?

Share this post


Link to post
Share on other sites
this is my current use for displayitems:

void character::displayitems()
{

vector<item> itemss=i.getitems();
if(itemss.empty())
{
cout<<"you are carrying nothing."<<endl;
return;
}

cout<<"you have:"<<endl;
for_each(itemss.begin(),itemss.end(),print);

}



and here is getitems:

vector<item> inventory::getitems()
{
return items;
}

Share this post


Link to post
Share on other sites
OK I can't see anything wrong with the code you have posted. Can we have a look at the operator>>(iostream, item) function?

Share this post


Link to post
Share on other sites
I'm really sorry I am not being particularly helpful so far.

I am wondering about the name field, in particular I am wondering if perhaps it does not get initialised properly. Something to try to see if I am on the right track is to alter the code for the operator<< like so:

ostream& operator << (ostream& os, const item& s)
{
os << "I am an item!";
return os;
}



If this runs successfully then it is something related to that field otherwise I am way off the mark.

Share this post


Link to post
Share on other sites
You really shouldn't be throwing your item vector around like you are (making copies), it is very resource intensive and isn't very practical for accessing your item container. I suggest returning a reference to your items:

std::vector<item>& inventory::getitems()
{
return items;
}

Like some other poster previously mentioned, your first and obvious problem was your use of the copied vector (using a copied vector to get the beginning iterator and then using another copied vector to get the end ).

Now the only problem I see is your direct implication of cout with the << operator when the output stream isn't necessarily cout.

Use:

ostream& operator << (ostream& os, const item& s)
{
os << s.name;
return os;
}


Share this post


Link to post
Share on other sites
Mmm. The item list isn't really the "property" of others. If you insist on doing it that way, might I at least suggest a bit of const correctness:


const std::vector<item>& inventory::getitems() const


This way, you will be prevented from writing something like i.getitems()[3] = item(); // omg hax!. The first const says "don't let people change the items after requesting them" (although they have ways around that). The second one says "(because people are not allowed to change the items), I warrant that the overall inventory object will not be changed by this call".

You might instead consider doing printing within the inventory, or if you want better generality, a templated delegation method that accepts a function to apply to each item - something like:


// You are the weakest link - goodbye.
//vector&lt;item&gt; inventory::getitems()
//{
// return items;
//}
// Instead:
template &lt;typename functor&gt;
void inventory::doWithEverything(const functor& f) {
std::for_each(items.begin(),items.end(),f);
}
bool inventory::empty() {
return items.empty();
}
// And then...
void character::displayitems() {
if(i.empty()) {
cout << "you are carrying nothing." << endl;
} else { // The "guard clause" approach is ok, but kinda silly for something this short
cout << "you have:" << endl;
i.doWithEverything(print);
}
// Although as SKonen says, you should usually prefer to generalize things
// like this, by accepting the stream as a parameter rather than
// assuming cout.
}



See? Now you still have "general purpose manipulation", but the icky internal details of dealing with the storage vector are confined to the appropriate place - so users of the inventory now don't have to worry about it.

This approach does let you pass in functors that would modify the items, but you won't be able to modify the vector itself from outside (i.e. reassign an item outright, or add an item) except in ways controlled by the inventory's interface. If you want to make the individual items const as well, things get a bit trickier. The only solutions I can think of are to store a vector<const item> (and use const_casts within the inventory implementation where required - evil, but shouldn't be frequently if at all necessary anyway), or use (really ungodly and not at all recommended even for sane experienced programmers) "introspection" hacks to check whether the "functor" method accepts a const item or not.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!