Need help with efficiency and elegance in simple code

[62BJW]

My code works but I don't believe it's as efficient as it could be. Specifically the Try/catch area of the code. If I don't try/catch each line separately it misses some of the doors. Like to reduce the regeneration time too if possible. Any pointers are appreciated. Thanks.

            // Get doors
            FilteredElementCollector collector = new FilteredElementCollector(doc);
            List<Element> coll = collector.OfClass(typeof(FamilyInstance))
                .OfCategory(BuiltInCategory.OST_Doors)
                .ToList();

            // Filtered element collector is iterable
            foreach (Element e in coll)
            {
                // Get the parameter name
                Parameter s_parameter = e.LookupParameter("Swing Angle");
                Parameter s1_parameter = e.LookupParameter("Swing Angle_Door 1");
                Parameter s2_parameter = e.LookupParameter("Swing Angle_Door 2");
                using (Transaction t = new Transaction(doc, "parameters"))

                // Modify document within a transaction
                using (Transaction tx = new Transaction(doc))
                {
                    tx.Start("Change door swing angles to 45");
                   
                    try
                    {
                        s_parameter.Set(0.785398163);
                    }
                    catch { }
                    try
                    {
                        s1_parameter.Set(0.785398163);
                    }
                    catch { }
                    try
                    { 
                        s2_parameter.Set(0.785398163);
                    }
                    catch { }
                    tx.Commit();
                }
            }
            TaskDialog.Show("Completed", "Door swings changed to 45°.");
            return Result.Succeeded;

[MarryTookMyCoffe]

1)change list to array, you don't have to change foreach to for because compiler usually do it for you, but remember that with out optimization of code for is faster than a foreach.

2) delete

using (Transaction t = new Transaction(doc, "parameters"))

why this is even here ? you put using transaction in to using transaction, why?

3) put loop in side transaction not out side(every start and commit take a lot of time)

4) if you get parameter with string property, you have to check if parameter is not null

if(s_parameter != null)

5)check if parameter is read only and if it takes double as value

6)so application can only Succeeded

7) do you change type or instance? you can .WhereElementIsElementType to a FillteredElementCollector

[62BJW]

Thanks for the reply. I understand some of what you said. I'm pretty new to C#. I use to write VB but that was a few years ago. I can't seem to move the start/commit outside of the loop. See image.

[MarryTookMyCoffe · Accepted solution]

you forget about {} after using, try this:

    [Transaction(TransactionMode.Manual)]
    public class Command : IExternalCommand
    {
        private const double angle = 0.785398163;
        public Result Execute(
          ExternalCommandData commandData,
          ref string message,
          ElementSet elements)
        {
            Document doc = commandData.Application.ActiveUIDocument.Document;
            // Get insatnce of doors
            Element[] coll = new FilteredElementCollector(doc).OfClass(typeof(FamilyInstance)).OfCategory(BuiltInCategory.OST_Doors).WhereElementIsNotElementType().ToArray();
            //start transaction out side of loop, that way you only open transaction once
            using (Transaction tx = new Transaction(doc))
            {
                tx.Start("Change door swing angles to 45");
                foreach (Element e in coll)
                {
                    //set Parameter by the name
                    try
                    {
                        SetParameter(e, "Swing Angle", angle);
                        SetParameter(e, "Swing Angle_Door 1", angle);
                        SetParameter(e, "Swing Angle_Door 2", angle);
                    }
                    catch { }
                }
                tx.Commit();
            }
            TaskDialog.Show("Completed", "Door swings changed to 45°.");
            return Result.Succeeded;
        }
        /// <summary>
        /// set double parameter by value
        /// </summary>
        /// <param name="element"></param>
        /// <param name="Name"></param>
        /// <param name="value"></param>
        /// <returns></returns>
        private static bool SetParameter(Element element, string Name, double value)
        {
            Parameter parameter = element.LookupParameter(Name);
            //preventing exceptions is better than catching it 
            if (parameter != null && !parameter.IsReadOnly && parameter.StorageType == StorageType.Double)
                return parameter.Set(value);
            return false;
        }
    }

[62BJW]

Thanks works perfectly. Now I'll step through the code so I can learn what you did. Thanks Again!

[jeremytammik]

Dear Bernie,

Thank you for your query.

I am very glad you care about these aspects.

Very many thanks to MarryTookMyCoffe for jumping in and helping so efficiently!

I would like to add a few points more:

LookupParameter will only retrieve the first parameter of the given name. In order to use it, you must be absolutely certain that only one parameter with the given name exists. I would suggest adding a test somewhere in your add-in startup code to ensure that this really is the case.

It is always safer to use other means to retrieve parameters, e.g., the built-in parameter enumeration value, if it exists, or simply the Parameter definition, which you can look up and cache beforehand, e.g., in the afore-mentioned code ensuring that the parameter name on that element type really is unique.

There is no need to convert the element collection to an array, i.e., saying

  Element[] coll = new FilteredElementCollector(doc)...ToArray();

You can perfectly well leave it as a filtered element collector and iterate directly over that instead:

  FilteredElementCollector coll = new FilteredElementCollector(doc)...;
  foreach (Element e in coll) ...

That will save conversion time and space and avoid the unnecessary data duplication.

I hope this helps.

Best regards,

Jeremy