Sign in to follow this  

Is this code bad?

This topic is 4199 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

What is your opion of this code? Is it unsafe? Can something similar be done using STL? ALLOCATOR is a class that stores any type of object //*************************************************************** enum LIST_TYPE { BASE, ITEM, SOLDIER, NUMTYPES }; // Lots of lists with different types all inheriting from the base class ENTITY ALLOCATOR <ENTITY>* Base; ALLOCATOR <ITEM_ENTITY>* ItemEntity; ALLOCATOR <SOLDIER_ENTITY>* Soldier; // One list to hold them all! ALLOCATOR <ENTITY>* List[NUMTYPES]; //*************************************************************** //*************************************************************** // Constructor code //*************************************************************** Base = new ALLOCATOR <ENTITY> (1); List[BASE] = Base; ItemEntity = new ALLOCATOR <ITEM_ENTITY> (1); List[ITEM] = reinterpret_cast <ALLOCATOR <ENTITY>*> (ItemEntity); Soldier = new ALLOCATOR <SOLDIER_ENTITY> (1); List[SOLDIER] = reinterpret_cast <ALLOCATOR <ENTITY>*> (Soldier); //***************************************************************

Share this post


Link to post
Share on other sites
It is a list class. You can store any type of item in it and check the contents later.

What I'm really interested in is this line:

List[ITEM] = reinterpret_cast <ALLOCATOR <ENTITY>*> (ItemEntity);


It compiles and runs but will it crash later?

Here's a bit more info on ALLOCATOR:

template <typename ITEM>
class ALLOCATOR
{
private:
int MaxItems;
int NumberAllocated;// Never decrement, this is the number of items allocated
ITEM** Items;
TAG_LIST* Tag;

Share this post


Link to post
Share on other sites
Have you thought about creating an abstract base class that you can derive those objects?

That way you can just make an array of that base class and not worry about using the template. You will only need to reinterpret_cast items in the array if you're trying to call a unique function to that class.

Share this post


Link to post
Share on other sites
Quote:
Original post by Davaris
What is your opion of this code?


You have an enumeration that is supposed to keep track of object types. That immediately raises a red flag.

Quote:
Is it unsafe?


You are casting between unrelated types: even if A and B are related, Foo<A> and Foo<B> generally aren't.

Quote:
Can something similar be done using STL?


Between the C++ Standard Library and the Boost libraries, probably.

But you should first try to explain carefully how you plan on using the pointers that you have stored in List.

Quote:
It compiles and runs but will it crash later?


Depends on what you do later. The only thing you can safely do is cast the pointer back to what it was originally. Trying to use the pointers that are in List directly may lead to Bad Things happening.

Share this post


Link to post
Share on other sites
In general reinterpret_cast should be treated as a last resort. There are usually better ways of accomplishing the same thing. Certainly you shouldn't be using them all over the place!

In this case I think what you are doing is wrong. I don't really see the point of having an array where each index contains a specific type of list. I hardly think there would be any need to iterate through this array. Even if you do, then the code to do it would be no longer if they were separate variables, given that you have to cast each one to the correct type anyway. It seems to me like they should just be separate lists.

No doubt, what you want to achieve with those 'lists' can be done much better using the C++ standard library.

Share this post


Link to post
Share on other sites
So how would you guys do it in STL?

I have many lists for each object type and they all inherit from the ENTITY class.

ALLOCATOR <ENTITY>* Base;
ALLOCATOR <ITEM_ENTITY>* ItemEntity;
ALLOCATOR <SOLDIER_ENTITY>* Soldier;
ALLOCATOR <PROJECTILE_ENTITY>* Projectile;
ALLOCATOR <CYCLE_ENTITY>* Cycle;
ALLOCATOR <BINARY_ENTITY>* Binary;
ALLOCATOR <DOOR_ENTITY>* Door;
ALLOCATOR <CONTAINER_MAP_ENTITY>* ContainerMap;
ALLOCATOR <TRIGGER_ENTITY>* Trigger;
ALLOCATOR <VEHICLE_ENTITY>* Vehicle;

What I want to do is use functions like:

ENTITY* Spawn (const int e_type = ENTITY::BASE)
{
ENTITY* e;

switch (type)
{
case ENTITY::BASE: e = Base->GetNewItem(); break;
case ENTITY::SOLDIER: e = Soldier->GetNewItem(); break;
// etc...
}
}

// and

ENTITY* Select (const int e_type, const int scrn_x, const int scrn_y)
{
// Search through List[e_type] for a match...
}

If I have to search through the lists separately (Base, ItemEntity, Soldier...) I wouldn't be able to write a general purpose search routine for all of the lists.


Share this post


Link to post
Share on other sites
Virtual functions and a factory. Without knowing more about your design:
class Entity
{

public:

virtual ~Entity();
virtual void select() = 0;

};

class Item
:
public Entity
{

public:

virtual void select()
{
// implementation
}

};

class Soldier
:
public Entity
{

public:

virtual void select()
{
// implementation
}

};

class EntityFactory
{

// your preferred factory implementation

};

// ...

boost::ptr_list< Entity > objects;
objects.push_back(EntityFactory::create(object_type, parameters));
std::for_each(objects.begin(), objects.end(), std::mem_fun_ref(&Entity::select));


Note that ALL_CAPS_WITH_UNDERSCORES is reserved by most coding standards for preprocessor definitions. Using it for class names is probably A Bad Idea™.

EDIT: I originally omitted the Entity destructor because the code was intended as an outline, not a solution. On reflection I decided it was better to include it.

Σnigma

[Edited by - Enigma on June 15, 2006 10:46:23 AM]

Share this post


Link to post
Share on other sites
>Note that ALL_CAPS_WITH_UNDERSCORES is reserved by most coding standards for preprocessor definitions. Using it for class names is probably A Bad Idea™.

Yes this is a very old engine. I'm trying to bring it and myself out of the dark ages. :)

So I have to get Boost as well? Man I've got a lot to learn. :)

You put everything in the same list. So when I destroy something I'd have to delete it. Would there be any problems with memory fragmentation? Hmmm... Perhaps I could add another variable to the entity class indicating whether or not it is in use.

Share this post


Link to post
Share on other sites

This topic is 4199 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.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this