Will the best code reviewer please stand up

[brianchapmandesign]

Just hoping someone will review my code below and offer advice for improvements. Basically want to learn. Thanks!!!

// send wipeouts to back

namespace BCCWIPEBACK
{
    public class DraftingTools
    {

        [CommandMethod("BCC:WBACK")]

        public static void BCCWIPETOBACK()
        {
            Document doc = Application.DocumentManager.MdiActiveDocument;
            Database db = doc.Database;
            Transaction tr = db.TransactionManager.StartTransaction();

            using (tr)
            {
                BlockTable bt = tr.GetObject(db.BlockTableId, OpenMode.ForWrite) as BlockTable;
                foreach (ObjectId objId in bt)
                {
                    BlockTableRecord btr = objId.GetObject(OpenMode.ForWrite) as BlockTableRecord;

                    foreach (ObjectId btrObjId in btr)
                    {
                        Entity ent = btrObjId.GetObject(OpenMode.ForWrite) as Entity;
                        if (ent is Wipeout)
                        {
                            DrawOrderTable drawOrder = tr.GetObject(btr.DrawOrderTableId, OpenMode.ForWrite) as DrawOrderTable;
                            ObjectIdCollection ids = new ObjectIdCollection();
                            ids.Add(ent.ObjectId);
                            drawOrder.MoveToBottom(ids);  //move the selected entity so that entity is drawn in the beginning of the draw order.
                        }
                    }
                } tr.Commit();
            }
        }
    }
}

[Alexander.Rivilis · Accepted solution]

Please compare:

namespace BCCWIPEBACK
{
  public class DraftingTools
  {
    [CommandMethod("BCC:WBACK")]
    public static void BCCWIPETOBACK()
    {
      Document doc = Application.DocumentManager.MdiActiveDocument;
      Database db = doc.Database;
      using (Transaction tr = db.TransactionManager.StartTransaction()) {
        BlockTable bt = tr.GetObject(db.BlockTableId, OpenMode.ForRead) as BlockTable;
        if (bt != null) {
          foreach (ObjectId objId in bt) {
            BlockTableRecord btr = objId.GetObject(OpenMode.ForRead) as BlockTableRecord;
            if (btr != null) {
              ObjectIdCollection ids = new ObjectIdCollection();
              foreach (ObjectId btrObjId in btr) {
                if (btrObjId.ObjectClass.IsDerivedFrom(RXClass.GetClass(typeof(Wipeout)))) {
                  ids.Add(btrObjId);
                }
                //////////////////////////////////////////////////////////////////////////////////
                //  Old versions of AutoCAD .NET API has not ObjectId.ObjectClass method. So:
                //////////////////////////////////////////////////////////////////////////////////
//                Entity ent = btrObjId.GetObject(OpenMode.ForRead) as Entity;
//                if (ent != null && ent is Wipeout) {
//                  ids.Add(btrObjId);
//                }
              }
              if (ids.Count > 0) {
                DrawOrderTable drawOrder = 
                   tr.GetObject(btr.DrawOrderTableId, OpenMode.ForWrite) as DrawOrderTable;
                if (drawOrder != null) {
                  // Why MoveToBottom()? In this case wipeouts
                  // do not hide entities. Maybe use MoveToTop() instead?
                  // drawOrder.MoveToTop(ids);  
                  drawOrder.MoveToBottom(ids);
                }
              }
            }
          } 
        }
        tr.Commit();
      }
      doc.Editor.Regen();
    }
  }
}

[brianchapmandesign]

Thanks Alexander (Alex ok?)!  You are awesome!

I really appreciate it... the "if" statements obviously stand out (on my phone that is lol), but will definately study it more the next few days.

Really appreciate your help.

[Alexander.Rivilis]

---

@bcinnv wrote:

...Alex ok?...

---

OK!

[BlackBox_]

Hi Alexander,

Would it not be better to use StartOpenCloseTransaction() in lieu of StartTransaction()?

Also, since the Database is being modified, shouldn't a DocumentLock wrap the Transaction's using statement?

[Alexander.Rivilis]

---

Would it not be better to use StartOpenCloseTransaction() in lieu of StartTransaction()?

---

This is a complex question. Possible and so and so. My preference - native ObjectARX and smart pointers.

---

Also, since the Database is being modified, shouldn't a DocumentLock wrap the Transaction's using statement?

---

No. As far as this function is the command handler, and this command working in document context (has not CommandFlag.Session flag) In this case, AutoCAD itself locks the document before command start and unlocks after its stop.

[BlackBox_]

Thanks for that clarification, Alexander... I did have a separate question, though, if I may?

Why iterate the entire BlockTable, when one could iterate a simple SelectionSet?

Pseudo-code (tested):

using Autodesk.AutoCAD.ApplicationServices;
using Autodesk.AutoCAD.DatabaseServices;
using Autodesk.AutoCAD.EditorInput;
using Autodesk.AutoCAD.Runtime;

using acApp = Autodesk.AutoCAD.ApplicationServices.Application;

namespace Autodesk.Forums.PseudoCode.DraftingTools
{
    public class Commands
    {
        [CommandMethod("WBACK")]
        public static void WipeOutToBack()
        {
            Document doc = acApp.DocumentManager.MdiActiveDocument;
            Database db = doc.Database;
            Editor ed = doc.Editor;

            TypedValue[] tvs = new TypedValue[]
            {
                new TypedValue((int)DxfCode.Start, "WIPEOUT")
            };

            SelectionFilter filter = new SelectionFilter(tvs);

            PromptSelectionResult psr = ed.SelectAll(filter);

            if (psr.Status == PromptStatus.OK)
            {
                using (Transaction trans =
                        db.TransactionManager.StartOpenCloseTransaction())
                {
                    SelectionSet ss = psr.Value;

                    ObjectId[] ids = ss.GetObjectIds();

                    foreach (ObjectId id in ids)
                    {
                        Entity ent =
                            trans.GetObject(id, OpenMode.ForRead) as Entity;

                        BlockTableRecord btr =
                            trans.GetObject(ent.BlockId, OpenMode.ForRead) as BlockTableRecord;

                        DrawOrderTable drawOrder =
                            trans.GetObject(btr.DrawOrderTableId, OpenMode.ForWrite) as DrawOrderTable;

                        ObjectIdCollection idToMove = new ObjectIdCollection();

                        idToMove.Add(id);

                        drawOrder.MoveToBottom(idToMove);
                    }

                    ed.WriteMessage("\n** Wipeout(s) sent to back ** ");

                    trans.Commit();
                }
            }

            else
                ed.WriteMessage("\n** No wipeout(s) found ** ");
        }
    }
}

[DiningPhilosopher]

---

@BlackBoxCAD wrote:

Thanks for that clarification, Alexander... I did have a separate question, though, if I may?

Why iterate the entire BlockTable, when one could iterate a simple SelectionSet?

Pseudo-code (tested):......

<snip> 

---

You might want to review your code.

First, you open the same DrawOrderTable for every entity in the selection.

Second, you are calling MoveToBottom() once for every entity in the selection.

Third, you are not modifying the WIPEOUT objects in the selection, and don't need to open them in the first place, because they are all WIPEOUT objects.

So....

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Autodesk.AutoCAD.Runtime;
using Autodesk.AutoCAD.DatabaseServices;
using Autodesk.AutoCAD.EditorInput;

namespace Autodesk.AutoCAD.ApplicationServices.MyExtensions
{
   public static class WipeOutOrderExample
   {
      [CommandMethod( "WIPEOUTORDER" )]
      public static void WipeOutOrderCommand()
      {
         Document doc = Application.DocumentManager.MdiActiveDocument;
         Editor ed = doc.Editor;

         TypedValue[] array = new TypedValue[] {
            new TypedValue(0, "WIPEOUT")};

         PromptSelectionResult psr = ed.SelectAll( new SelectionFilter( array ) );
         if( psr.Status != PromptStatus.OK || psr.Value.Count == 0 )
         {
            ed.WriteMessage( "\nNo editable WIPEOUT objects found." );
            return;
         }

         ed.WriteMessage( "\n{0} WIPEOUTs found,", psr.Value.Count );
         PromptKeywordOptions pko = new PromptKeywordOptions( "\nMove to Top or Bottom?" );
         pko.Keywords.Add( "Top" );
         pko.Keywords.Add( "Bottom" );
         pko.Keywords.Default = "Bottom";
         PromptResult pr = ed.GetKeywords( pko );
         if( pr.Status != PromptStatus.OK )
            return;

         bool top = pr.StringResult == "Top";

         using( Transaction tr = doc.TransactionManager.StartTransaction() )
         {
            ObjectIdCollection ids = new ObjectIdCollection( psr.Value.GetObjectIds() );

            BlockTableRecord btr = (BlockTableRecord)
               tr.GetObject( doc.Database.CurrentSpaceId, OpenMode.ForRead );

            DrawOrderTable dot = (DrawOrderTable)
               tr.GetObject( btr.DrawOrderTableId, OpenMode.ForWrite );

            if( top )
               dot.MoveToTop( ids );
            else
               dot.MoveToBottom( ids );

            tr.Commit();
            doc.Editor.WriteMessage( "Modified draw order of {0} WIPEOUTs", ids.Count );
         }
      }
   }
}

[brianchapmandesign]

Thank you! sorry... I should clarify the purpose of the code is to deal with a glitch I've had since I moved to windows 7 / 64bit system, and Civil 3d. Wipeouts randomly jump to the top of the objects. Such as textmasks. Instead of hiding what's under the text...the draw order randomly reverses, and hides everything. Often it's a problem due to locked layers, but that's not always the case.

When I manually set the draw order for the wipeouts by selecting them all, I no longer have that problem.  What I've been doing is sending the wipeouts to the back, then my xrefs. It's worked well so far. 

Sorry for the confusion.

[BlackBox_]

---

@DiningPhilosopher wrote:

---

---

You might want to review your code.

First, you open the same DrawOrderTable for every entity in the selection.

Second, you are calling MoveToBottom() once for every entity in the selection.

...

//<snip>

---

How very inefficient of me (oops); thanks for the correction....

That was a kludge thrown together for me to learn from (and that I have, thank you) right as I was leaving the office, as every single ADN DevBlog example only showed how to modify the draw order for single entity selection... Couldn't find one modifying SelectionSet.

Admittedly, I overlooked passing SelectionSet.GetObjectIds() to DrawOrderTable.MoveTo*(). Thanks for the clarification, and code as an example.

Now that that's out of the way, I am correct in that (logically) it is better to perform this task on a valid SelectionSet, rather than iterating the BlockTable, no?

Cheers!

Edit to add:

"Third, you are not modifying the WIPEOUT objects in the selection, and don't need to open them in the first place, because they are all WIPEOUT objects."

The code I posted (never mind the duplication of work) _did_ modify the Wipeout entities (not express tools text masks, WIpeouts)... My test was performed on a line drawn with a Wipeout drawn over the top such that part of the line was no longer visible. The code (albeit innefficiently) correctly moved the Wipeout(s) to the back so that the line was fully visible.

If I've misunderstood your point, please clarify.

[DiningPhilosopher]

---

@BlackBoxCAD wrote:

Now that that's out of the way, I am correct in that (logically) it is better to perform this task on a valid SelectionSet, rather than iterating the BlockTable, no?

Cheers!

Edit to add:

"Third, you are not modifying the WIPEOUT objects in the selection, and don't need to open them in the first place, because they are all WIPEOUT objects."

The code I posted (never mind the duplication of work) _did_ modify the Wipeout entities (not express tools text masks, WIpeouts)... My test was performed on a line drawn with a Wipeout drawn over the top such that part of the line was no longer visible. The code (albeit innefficiently) correctly moved the Wipeout(s) to the back so that the line was fully visible.

If I've misunderstood your point, please clarify.

---

On the first question, if you're working in the drawing editor and operating on objects in the current space, then it makes sense to use SelectAll() with a filter. Of course, sometimes we want our code to work with databases that aren't open in the editor, which precludes using object selection or selection filtering, and requires that we iterate over BlockTableRecord objects directly and operate on those objects of interest, as Alex's code does. So, the answer is 'It depends".

In the code you posted, the WIPEOUT objects are not modified. You are opening them with OpenMode.ForRead.

To modify the draw order of an object, you don't have to modify the object, only the DrawOrderTable.

[BlackBox_]

To the first point; I fully understand this... Been using ObjectDBX (LISP) for years. I fully agree.

To the second part, many thanks for the clarification... First time in the draw order water; greatly appreciate the education.

Cheers!

[DiningPhilosopher]

One minor problem with the sample I posted, is that SelectAll() selects objects in both model and paper space, which is a problem since the code is operating only on the DrawOrderTable of the current space.

Here's the corrected version that gets around that problem:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Autodesk.AutoCAD.Runtime;
using Autodesk.AutoCAD.DatabaseServices;
using Autodesk.AutoCAD.EditorInput;

namespace Autodesk.AutoCAD.ApplicationServices.MyExtensions
{
   public static class WipeOutOrderExample
   {
      [CommandMethod( "WIPEOUTORDER" )]
      public static void WipeoutOrderCommand()
      {
         Document doc = Application.DocumentManager.MdiActiveDocument;
         Editor ed = doc.Editor;

         using( Transaction tr = doc.Database.TransactionManager.StartTransaction() )
         {

            BlockTableRecord btr = (BlockTableRecord)
               tr.GetObject( doc.Database.CurrentSpaceId, OpenMode.ForRead );
               
            Layout layout = (Layout) tr.GetObject( btr.LayoutId, OpenMode.ForRead );

            TypedValue[] array = new TypedValue[] {
               new TypedValue( 0, "WIPEOUT" ),
               new TypedValue( 410, layout.LayoutName ) 
            };

            PromptSelectionResult psr = ed.SelectAll( new SelectionFilter( array ) );

            if( psr.Status != PromptStatus.OK || psr.Value == null || psr.Value.Count == 0 )
            {
               ed.WriteMessage( "\nNo WIPEOUT objects found in current space." );
               tr.Commit();
               return;
            }

            ed.WriteMessage( "\n{0} WIPEOUTs found,", psr.Value.Count );
            PromptKeywordOptions pko = new PromptKeywordOptions( "\nMove to Top or Bottom?" );
            pko.Keywords.Add( "Top" );
            pko.Keywords.Add( "Bottom" );
            pko.Keywords.Default = "Bottom";
            PromptResult pr = ed.GetKeywords( pko );
            if( pr.Status != PromptStatus.OK )
            {
               tr.Commit();
               return;
            }

            bool top = pr.StringResult == "Top";

            ObjectIdCollection ids = new ObjectIdCollection( psr.Value.GetObjectIds() );

            DrawOrderTable dot = (DrawOrderTable)
               tr.GetObject( btr.DrawOrderTableId, OpenMode.ForWrite );

            if( top )
               dot.MoveToTop( ids );
            else
               dot.MoveToBottom( ids );

            tr.Commit();
            doc.Editor.WriteMessage( "Modified draw order of {0} WIPEOUTs", ids.Count );
         }
      }
   }
}

[Alexander.Rivilis]

---

@BlackBoxCAD wrote:
Why iterate the entire BlockTable, when one could iterate a simple SelectionSet?

---

In addition to  Tony (@DiningPhilosopher  posts, I would like say that with SelectionSet you can not select entities within the block definition. So if you have to process ALL wipeouts in Database - you have to iterate the entire BlockTable (there is another way, but it looks more like a hack).

[jeff]

First off I am far from the best code reviewer and secondly it is easier for my to type sitting down instead of standing up.

No need to open everything for write.

For a little info on casting,

You have your explicit cast -> (string)word.....

which will try to cast and either succeed or throw an InvalidCastException.

If you look at the 'is' operator in IL it tries the explicit cast then compares result with null.

The  'as' operator will return null instead of throwing an InvalidCastException.

So the is operator tries the cast and if it succeeds it throws away the casted result and returns true and then turn around and  cast it again, but with as operator it preserves it or returns null.

Depending on how the 'as' operator is used you can  prevent yourself from knowing if it was a failed cast or from passing a null value as the argument.

Some of the guys put wipeouts inside alot of their block definitions and use them for covering up objects under the "open areas" of a block insertion(A example is like a keyed not in a busy area with a wipeout under circle or hexagon and attribute).

I do not know what causes it and just pops up here and there, but for some reason the wipeout makes it way up to the front and starts covering up the entites in the block definition.

Obviously Alex and Tony know better than i do but I do not see how using SelectAll is any better especially in this case.

Do you guys perfer calling SelectAll or iterating yourself? 

I thought it was more efficient and flexible to iterate and filter yourself instead of SelectAll which has to do the same thing?

In this case the only object being open for write is the DrawOrderTable so no benefit of filtering for entities on locked layers, and where you are not asking user for a selection,

would you guys just iterate yourself?

[DiningPhilosopher]

If you need to move wipeouts in any block or all blocks, there's no choice.

The code I posted was simply to show a more-efficient version of what BlockBox posted, to illustrate the comments I had on that. But, what that code does could be done by one line of LISP.

[BlackBox_]

I really appreciate the discussion, and the clarifying of different points of view (i.e., how others utilize wipeouts, etc.)... Very informative, and prompts me to factor in other considerations when developing for myself, or attempting to help others.

This has been very educational, gentlemen - Cheers!

[jeff]

I gotcha there Tony I just meant in general if you had to filter entites within the current space and were not prompting for user to select which method you perfered using SelectAll or iterating yourself.

Dug up what I had laying around for personal use that I put in certain projects until I figure out if it is useful or how to bettter implement it.

Thanks again to Tony as it uses ideas expressed by him and from code examples he has posted.

        [CommandMethod("WipeOutToBack")]
        public void WipeOutToBack()
        {
            using (Transaction trx = Doc.TransactionManager.StartTransaction())
            {
                BlockTable bt = Db.BlockTable();
                var blks = bt.GetBlockTableRecords().LocallyDefinedBlocks();

                foreach (BlockTableRecord btr in blks)
                {
                    ObjectId[] idArr = btr.GetObjectIds<Wipeout>().ToArray();
                    ObjectIdCollection ids = new ObjectIdCollection(idArr);
                    if (ids.Count > 0)
                    {
                        DrawOrderTable dot = btr.DrawOrderTableId.GetDBObject<DrawOrderTable>(OpenMode.ForWrite);
                        dot.MoveToBottom(ids);
                    }
                }

                trx.Commit();
            }

            Ed.Regen();
        }

 Extenion methods should be self-explainatory except for maybe ones below

public static IEnumerable<ObjectId> GetObjectIds<T>(this BlockTableRecord btr) where T : Entity
        {
            IntPtr impobj = RXClass.GetClass(typeof(T)).UnmanagedObject;
            foreach (ObjectId id in btr)
            {
                if (id.ObjectClass.UnmanagedObject == impobj)
                {
                    yield return id;
                }
            }
        }

        /// <summary>
        /// Author: Tony Tanzillo
        /// Source: http://www.theswamp.org/index.php?topic=41311.msg464529#msg464529
        /// </summary>
        /// <param name="source"></param>
        /// <returns></returns>
        public static IEnumerable<BlockTableRecord> UserDefinedBlocks(this IEnumerable<BlockTableRecord> source)
        {
            return source.Where(btr =>
                !(
                btr.IsDependent ||
                btr.IsAnonymous ||
                btr.IsFromExternalReference ||
                btr.IsFromOverlayReference ||
                btr.IsLayout ||
                btr.IsAProxy
                )
                );
        }

        /// <summary>
        /// Author: Tony Tanzillo
        /// Source: http://www.theswamp.org/index.php?topic=41311.msg464529#msg464529
        /// Use instead of UserDefinedBlocks when anonymous blocks are needed
        /// </summary>
        /// <param name="source"></param>
        /// <returns></returns>
        public static IEnumerable<BlockTableRecord> LocallyDefinedBlocks(this IEnumerable<BlockTableRecord> source)
        {
            return source.Where(btr =>
                !(
                btr.IsDependent ||
                btr.IsFromExternalReference ||
                btr.IsFromOverlayReference ||
                btr.IsLayout ||
                btr.IsAProxy
                )
                );
        }

[DiningPhilosopher]

I would use a SelectionSet with filtering when working on drawings open in the editor, but I would separate that from the code that does the work, so that it could be used with both an array of ObjectIds from SelectionGet.GetObjectIds(), or from the IEnumerator of a BlockTableRecord.

So, with the various LINQ extension methods I use, like GetObjects<T>( this IEnumerable<ObjectId>, ...), I can operate on a sequence of ObjectIds that come from a SelectionSet's GetObjectIds() method, or from BlockTableRecord.Cast<ObjectId>().

The important things for me are, separation, separation, and separation. I keep the code that operationes on DBObjects seperate from the code that produces them.