Unable to remove not placed rooms and unnecessary Areas from list collection

[Anonymous]

Hi everyone,

Quick question I have some code that collects all rooms but i want to remove all "Not Placed" and Area over 0 via the following "private void"

I have written the following code but it throws a transaction issue:

/// <summary> /// Remove rooms not placed on the drawing /// or have a total area equal or less than zero /// </summary>

private void RemoveNotPlacedAndZeroAreaRooms(List<Room> rooms)

{

foreach (Room room in rooms)

{

if(room.Area <= 0 || room.Location == null)

{

rooms.Remove(room);

}

}

}

Is this code correct? Or am I missing something elsewhere in the code?

Could you please help

Regards,

Simon Palmer

[desdinova]

Try 

 if(room.Area.ToString()=="0")

Works for me.

[Revitalizer]

Hi,

you must not modify a list while iterating over it.

Put the valid rooms into another list.

Revitalizer

[Dale.Bartlett]

Exactly. I have fallen into that trap many times. Sample pseudo-code:

private void RemoveNotPlacedAndZeroAreaRooms(List<Room> rooms)
{
  foreach (Room room in rooms)
  {
    // collect room ids to delete
    if(room.Area <= 0 || room.Location == null)
    {
       idsToDelete.Add(room.Id);
    }
  }
  // now delete
  doc.Delete(idsToDelete);
}

[BobbyC.Jones]

I like to use ienumerable extensions to clarify and simplify these filtering tasks.

 public static IEnumerable<Room> WhereIsPlacedAndEnclosed(this IEnumerable<Room> rooms)
 {
    //IsPlaced() and IsEnclosed() Room extensions would make this even better
    var placedRooms = rooms.Where(r => r.Area >= 0).Where(r => r.Location != null);

    return placedRooms;
 }

public Result Execute(ExternalCommandData commandData, ref string message, ElementSet elements)
{
    var activeDoc = commandData.Application.ActiveUIDocument.Document;
    var roomCollector = new FilteredElementCollector(activeDoc);

    var placedRooms = roomCollector.
        OfClass(typeof(SpatialElement)).
        OfType<Room>().
        WhereIsPlacedAndEnclosed();

    foreach (var room in placedRooms)
    {
        //do your thing.
    }

    return Result.Succeeded;
}

[BobbyC.Jones · Accepted solution]

This is a little off topic, but this code had a slight smell I thought I could ignore, but I was wrong 🙂

A Room that is not placed is by default also not enclosed.  So unless you must specifically differentiate between not placed and placed but not enclosed rooms, a single test for enclosed rooms should suffice for filtering.  That simplifies things down to an extension method for determining if a room IsEnclosed().  Then you could use it with the standard linq Where() method, and ditch the WhereIsPlacedAndEnclosed() method.

var enclosedRooms = roomCollector.
    OfClass(typeof(SpatialElement)).
    OfType<Room>().
    Where(r => r.IsEnclosed());

As for the implementation of the IsEnclosed() method, testing that the room.Area > 0 is *probably* OK, but testing this inequality without a tolerance is another code smell you may want to account for in your production code.

public static bool IsEnclosed(this Room room)
{
    var isEnclosed = room.Area > 0;

    return isEnclosed;
}

And just as I finish typing this and clearing my mind of niggling smells, my last PR has been approved and merged and I can get back to work.  If the rest of the day goes half as well then I'll consider this an early Good Friday 🙂