topic Re: Need help with efficiency and elegance in simple code in Revit API Forum

Need help with efficiency and elegance in simple code

62BJW — Mon, 19 Nov 2018 14:49:04 GMT

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;

Re: Need help with efficiency and elegance in simple code

MarryTookMyCoffe — Mon, 19 Nov 2018 16:03:47 GMT

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

Re: Need help with efficiency and elegance in simple code

62BJW — Mon, 19 Nov 2018 17:08:17 GMT

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.

Re: Need help with efficiency and elegance in simple code

MarryTookMyCoffe — Mon, 19 Nov 2018 17:52:20 GMT

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;
        }
    }

Re: Need help with efficiency and elegance in simple code

62BJW — Mon, 19 Nov 2018 18:19:08 GMT

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

Re: Need help with efficiency and elegance in simple code

jeremytammik — Tue, 20 Nov 2018 07:47:12 GMT

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