I have a class like this:
class BuildingFloor
{
// The list of building rooms;
// Room is "heavy" object with many-many fields
List< Room > m_rooms;
// The list of all apertures on the floor
List< Aperture > m_apertures;
...
List< Room > Rooms
{
get { return m_rooms; }
}
List< Aperture > Doors
{
get{ return GetAperturesOfType( 2 ); }
}
public List< Aperture > Exits
{
get{ return GetAperturesOfType( 3 ); }
}
...
List< Aperture > GetAperturesOfType( int type )
{
var apertures = new List<ApertureWrapper>();
foreach ( var aper in m_apertures )
{
if ( ( aper.Type == type ) )
{
apertures.Add( aper );
}
}
return apertures;
}
}
My questions are:
1) Will m_rooms
be copied when client code will access Rooms
property;
2) How many times List<>
object will be constructed on the Doors
property call.
So, what i can change in this code to make it faster?
I need to heavily use these properties in code like
foreach( var door in m_floor.Doors) { ... }
NOTE: Profiler says Exits
property spent significant amount of time. Proof:
Will m_rooms be copied when client code will access Rooms property
Yes. But the value of m_rooms
is just a reference - it's not a List<Room>
object in itself. You might want to read my article about reference types and value types.
How many times
List<>
object will be constructed on the Doors property call.
GetAperturesOfType
once, which constructs a new List<ApertureWrapper>
.Your property would be more simply implemented as:
return m_apertures.Where(x => x.Type == 2).ToList();
(It sounds like you might want an ApertureType
enum, too...)
Also note that if you just need to iterate over the doors, you could just use:
return m_apertures.Where(x => x.Type == 2);
and avoid creating a List<>
at all. That would have different semantics in other ways, mind you...
Profiler says Doors property spent significant amount of time.
Well you'd need to see how many apertures you've actually got, how many are doors, and how often you're calling the Doors
property. We can't really tell what's significant overall just from what you've shown us. Performance work is usually contextual.
EDIT: Now we've seen the calling code, it would be better if you used a local variable:
var exits = m_building.CurrentFloor.Exits;
for (int i = 0; i < exits.Count; i++)
{
// Use exits[i] within here
// Set exitId to i + 1 if you find the exit.
// (Or ideally, change everything to be 0-based...)
}
Otherwise you're creating a new list for each iteration of the loop.
See more on this question at Stackoverflow