Skip to main content
GameDev.net gamedev.net
🔒 Locked

STL List Question

Started by Swatter555 Apr 25, 2006 at 10:39 PM 15 replies 2.2k views
Original Post
Swatter555
Swatter555
I am wondering if I am using the stl lists correctly. I have the class Batt_List wrapped around the list. I use this method to retrieve an element from the list, for what Im thinking should be read/write. Batts are the objects stored in the list. 

//GetBattByID()
bool Batt_List::GetBattByID(int _param,Batt& battRef)
{
    //Check for a size of zero.
	if(GetSize() == 0) return false;

	//Check if unit is in list, set reference if so.
	for(iter = battList.begin(); iter != battList.end(); iter++)
	{
		//Set element if its in the list.
		if(iter->GetBattID() == _param) battRef = *iter;
	}//end for

	return true;
}
//GetBattByID()
I use it in this fashion. This works fine to extract info from the list, but I cannot change the data of the Batt object that is in the list. I dont know if Im confusing myself with pointers or if I have a bug somewhere else. 


Batt temp;

//getting the first object in the list for example
listObject.GetBattByID(0,temp);

//Change the data in temp here.
jflanglois
jflanglois
Well, it looks OK to me Shouldn't you be returning false if you didn't find the element? You also shouldn't name your parameters _param. It doesn't really help with readability.

This is a minor improvement, but you could break after setting your reference. You don't need to iterate through the rest of the list. Also, take a look at std::find_if for the "STL way".

The reason you are not able to change the item in the list is that you are making a copy to temp. If temp were of type Batt &, then it would do what you wanted.

[Edited by - jflanglois on April 25, 2006 11:06:56 PM]
Swatter555
Swatter555
Quote:
Original post by jflanglois
Well, it looks OK to me Shouldn't you be returning false if you didn't find the element? You also shouldn't name your parameters _param, though. It doesn't really help with readability.

This is a minor improvement, but you could break after setting your reference. You don't need to iterate through the rest of the list. Also, take a look at std::find_if for the "STL way".

The reason you are not able to change the item in the list is that you are making a copy to temp. If temp were of type Batt &, then it would do what you wanted.



Well, the element will be in there if size is not zero. I assign id nums when I add them to list. I just iterate from zero to size of list.

I had already put a return true after the element was found right after I posted the code.


Could you put some code in a window as how to properly get a Batt out of the list. Thanks :)
scott_l_smith
scott_l_smith
Although it has nothing to do with the correctness of your code, it is worth mentioning that it is good practice to always preincrement your iterators (that is, ++iter). The post-increment operator requires a copy of the iterator (or any object that overloads the operator) to be made, whereas the preincrement operator does not. For simple types like the list iterator the difference may be slight, but in a large list you are making many unnecessary copies (read: slow). Sorry to get off topic, but I thought it was worth mentioning.

-Scott
Swatter555
Swatter555
Quote:
Original post by scott_l_smith
Although it has nothing to do with the correctness of your code, it is worth mentioning that it is good practice to always preincrement your iterators (that is, ++iter). The post-increment operator requires a copy of the iterator (or any object that overloads the operator) to be made, whereas the preincrement operator does not. For simple types like the list iterator the difference may be slight, but in a large list you are making many unnecessary copies (read: slow). Sorry to get off topic, but I thought it was worth mentioning.

-Scott



Thanks, I will take that into consideration :)
jflanglois
jflanglois
Well, here is how I would do it:
#include <list>#include <functional>#include <algorithm>struct isID : public std::binary_function< int, Batt, bool > {  bool operator()( const int ID, const Batt &b ) const {    return b.GetBattID() == ID;  }};bool Batt_List::GetBattByID(int ID, Batt &batt) {  std::list< Batt >::iterator it = std::find_if( battList.begin(), battList.end(), std::bind1st( isID(), ID ) );  if( it == battList.end() )    return false;  batt = *it;  return true;}
But it's a little overkill for your example.


Quote:
Well, the element will be in there if size is not zero. I assign id nums when I add them to list. I just iterate from zero to size of list.
And what happens if _param is bigger that the size of your list? Indeed, why have an ID at all?
Swatter555
Swatter555
That struct isID is killing me :)

Your code is a notch above my understanding, a simplified version would be better.

The id stuff is a work in progress. The is my first try at lists, so Im sure Ill get better at it as I go.
jflanglois
jflanglois
Well, what exactly are you trying to do?

The simplified version of my code looks very much like your code:
bool Batt_List::GetBattByID( int ID, Batt &batt ) {  for( std::list< Batt >::iterator it = battList.begin(); it != battList.end(); ++it )    if( it->GetBattID() == ID ) {      batt = *it;      return true;    }  return false;}
[edit] Actually, this is clearer. If the condition for find_if were more complicated, then the first version becomes more useful (especially if the condition is often used).
Swatter555
Swatter555
I was having problems calling the method.

I was using it like:

Batt temp;

listObject.GetBattByID(0,temp);


Then you said:
"The reason you are not able to change the item in the list is that you are making a copy to temp. If temp were of type Batt &, then it would do what you wanted."

I cant get the syntax right for that.


jflanglois
jflanglois
It should be:

Batt &temp;
listObject.GetBattByID(0,temp);
iMalc
iMalc
I'm not convinced that a list is the right container to be using here.
What is the reason you aren't using a map instead?
Swatter555
Swatter555
Batt &temp Gives me the error that references must be initialized.

As far as a map goes, I am reading about that too. I am using list to store maybe up to 500 or so Batt objects. I am really just feeling around the STL at this point. I am not sure if a sequential container is the best to use at this point, but that is the kind of stuff I am testing.
MrEvil
MrEvil
Yes, references must be initialised. If you want to do it like that, then your function must return a reference, so that it can be used in an initialiser. This means you have to throw an exception if you didn't find it (because you can't just return a null reference).

struct batt_not_found{};Batt& Batt_List::GetBattByID(int ID) {  for( std::list< Batt >::iterator it = battList.begin(); it != battList.end(); ++it )    if( it->GetBattID() == ID )      return *it;  throw batt_not_found(); //maybe use a standard exception here}try{  Batt& temp = listObject.GetBattByID(0);  //use temp}catch(batt_not_found&){  //...handle exception :(}


Or could use a pointer.

Batt* Batt_List::GetBattByID(int ID) {  for( std::list< Batt >::iterator it = battList.begin(); it != battList.end(); ++it )    if( it->GetBattID() == ID ) {      return &*it; //I hate doing stuff like that because then I have to explain it    }  return false; }Batt* temp = listObject.GetBattByID(0);if(temp){  //use temp  /* //You  could even do:  Batt& batt = *temp;  */}


I prefer the pointer version in this case.



If you're doing a lot of GetBattByID()s, map might be a good choice here, especially considering it models the type of container you seem to want. For containers of size 500 as you suggested, it would provide much faster lookup. Furthermore, you wouldn't need to provide an ID member of Batt

Lookup isn't everything, of course.
jflanglois
jflanglois
Quote:
Original post by Swatter555
Batt &temp Gives me the error that references must be initialized.
Right, sorry about that. My mistake.

Mr.Evil's post has it right.
moeron
moeron
If you are doing a lot of lookups in your list you may want to use a data type that allows for faster searches

std::map
std::set

[edit]
whups, just realized Mr. Evil already said this...sorry =)
moe.ron
Swatter555
Swatter555
Thanks everyone, Ill investigate using map or set.
Zahlman
Zahlman
If you determine that the list is appropriate, and don't want to throw when not found, you could use a pointer instead of a reference, returning null when not found. (This is appropriate if it is expected that you will not find the object a significant amount of the time.)

Batt* Batt_List::getBattByID(int id) {  for (std::list< Batt >::iterator it = battList.begin(); it != battList.end(); ++it) {    if (it->GetBattID() == id)      // Dereference the iterator to get a reference; reference the reference to      // get a pointer.      return &(*it);    }  }  return 0;}

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.