Clean Code: Readable Dependency Injection suggestions?

Question

I have a project that adds elements to an AutoCad drawing. I noticed that I was starting to write the same ten lines of code in multiple methods (only showing two for simplicity).

Initial Implementation: You will notice that the only thing that really changes is adding a Line instead of a Circle.

[CommandMethod("Test", CommandFlags.Session)]
    public void Test()
    {
        AddLineToDrawing();
        AddCircleToDrawing();
    }

    private void AddLineToDrawing()
    {
        using (DocumentLock lockedDocument = Application.DocumentManager.MdiActiveDocument.LockDocument())
        {
            using (Database database = Application.DocumentManager.MdiActiveDocument.Database)
            {
                using (Transaction transaction = database.TransactionManager.StartTransaction())//Start the transaction
                {
                    //Open the block table for read
                    BlockTable blockTable = transaction.GetObject(database.BlockTableId, OpenMode.ForRead) as BlockTable;

                    //Open the block table record model space for write
                    BlockTableRecord blockTableRecord = (BlockTableRecord)transaction.GetObject(blockTable[BlockTableRecord.ModelSpace], OpenMode.ForWrite);

                    Line line = new Line(new Point3d(0, 0, 0), new Point3d(10, 10, 0));
                    blockTableRecord.AppendEntity(line);

                    transaction.AddNewlyCreatedDBObject(line, true);

                    transaction.Commit();
                }
            }
        }
    }

    private void AddCircleToDrawing()
    {
        using (DocumentLock lockedDocument = Application.DocumentManager.MdiActiveDocument.LockDocument())
        {
            using (Database database = Application.DocumentManager.MdiActiveDocument.Database)
            {
                using (Transaction transaction = database.TransactionManager.StartTransaction())//Start the transaction
                {
                    //Open the block table for read
                    BlockTable blockTable = transaction.GetObject(database.BlockTableId, OpenMode.ForRead) as BlockTable;

                    //Open the block table record model space for write
                    BlockTableRecord blockTableRecord = (BlockTableRecord)transaction.GetObject(blockTable[BlockTableRecord.ModelSpace], OpenMode.ForWrite);

                    Circle circle = new Circle(new Point3d(0, 0, 0), new Vector3d(0, 0, 0), 10);
                    blockTableRecord.AppendEntity(circle);

                    transaction.AddNewlyCreatedDBObject(circle, true);

                    transaction.Commit();
                }
            }
        }
    }

Injection: This approached removed the duplication of code, but I think the readability is poor.

[CommandMethod("Test", CommandFlags.Session)]
    public void Test()
    {
        PerformActionOnBlockTable(new CircleDrawer());
        PerformActionOnBlockTable(new LineDrawer());
    }

    public interface IDraw
    {
        DBObject DrawObject(BlockTableRecord blockTableRecord);
    }

    public class CircleDrawer : IDraw
    {
        public DBObject DrawObject(BlockTableRecord blockTableRecord)
        {
            Circle circle = new Circle(new Point3d(0, 0, 0), new Vector3d(0, 0, 0), 10);
            blockTableRecord.AppendEntity(circle);

            return circle;
        }
    }

    public class LineDrawer : IDraw
    {
        public DBObject DrawObject(BlockTableRecord blockTableRecord)
        {
            Line line = new Line(new Point3d(0, 0, 0), new Point3d(10, 10, 0));
            blockTableRecord.AppendEntity(line);

            return line;
        }
    }

    private void PerformActionOnBlockTable(IDraw drawer)
    {
        using (DocumentLock lockedDocument = Application.DocumentManager.MdiActiveDocument.LockDocument())
        {
            using (Database database = Application.DocumentManager.MdiActiveDocument.Database)
            {
                using (Transaction transaction = database.TransactionManager.StartTransaction())//Start the transaction
                {
                    //Open the block table for read
                    BlockTable blockTable = transaction.GetObject(database.BlockTableId, OpenMode.ForRead) as BlockTable;

                    //Open the block table record model space for write
                    BlockTableRecord blockTableRecord = (BlockTableRecord)transaction.GetObject(blockTable[BlockTableRecord.ModelSpace], OpenMode.ForWrite);

                    DBObject newObject = drawer.DrawObject(blockTableRecord);

                    transaction.AddNewlyCreatedDBObject(newObject, true);

                    transaction.Commit();
                }
            }
        }
    }

Injecting Func<>: This seemed to give me a similar result, with better readability.

[CommandMethod("Test", CommandFlags.Session)]
    public void Test()
    {
        PerformActionOnBlockTable(AddLineToDrawing);
        PerformActionOnBlockTable(AddCircleToDrawing);
    }

    private void PerformActionOnBlockTable(Func<BlockTableRecord, DBObject> action)
    {
        using (DocumentLock lockedDocument = Application.DocumentManager.MdiActiveDocument.LockDocument())
        {
            using (Database database = Application.DocumentManager.MdiActiveDocument.Database)
            {
                using (Transaction transaction = database.TransactionManager.StartTransaction())//Start the transaction
                {
                    //Open the block table for read
                    BlockTable blockTable = transaction.GetObject(database.BlockTableId, OpenMode.ForRead) as BlockTable;

                    //Open the block table record model space for write
                    BlockTableRecord blockTableRecord = (BlockTableRecord)transaction.GetObject(blockTable[BlockTableRecord.ModelSpace], OpenMode.ForWrite);

                    DBObject newObject = action(blockTableRecord);

                    transaction.AddNewlyCreatedDBObject(newObject, true);

                    transaction.Commit();
                }
            }
        }
    }

    private DBObject AddLineToDrawing(BlockTableRecord blockTableRecord)
    {
        Line line = new Line(new Point3d(0, 0, 0), new Point3d(10, 10, 0));
        blockTableRecord.AppendEntity(line);

        return line;
    }

    private DBObject AddCircleToDrawing(BlockTableRecord blockTableRecord)
    {
        Circle circle = new Circle(new Point3d(0, 0, 0), new Vector3d(0, 0, 0), 10);
        blockTableRecord.AppendEntity(circle);

        return circle;
    }

I can honestly say that I have not done much with DI, so I'm quite new to this. Could any of you more experienced developers give me Pro's/Con's of the two different approaches? Is there anything in the last approach that's a red flag? It seems to be more readable than the second approach. Maybe I'm not even completely understanding injection... Thanks in advance for you input!

Answer 1

You could do a simple refactoring instead of the options you provided:

[CommandMethod("Test", CommandFlags.Session)]   
public void Test() {   
  AddLineToDrawing();   
  AddCircleToDrawing();   
}  

private void AddLineToDrawing() {   
  CreateObjectOnBlockTable(
    new Line(new Point3d(0, 0, 0), new Point3d(10, 10, 0)));   
}   

private void AddCircleToDrawing() {   
  CreateObjectOnBlockTable(
    new Circle(new Point3d(0, 0, 0), new Vector3d(0, 0, 0), 10));   
}   

private void CreateObjectOnBlockTable(DBObject dbObject) { 
  using (var lockedDocument = Application.DocumentManager.MdiActiveDocument.LockDocument()) 
  using (var database = Application.DocumentManager.MdiActiveDocument.Database) 
  using (var transaction = database.TransactionManager.StartTransaction()) {
    // Open the block table for read 
    var blockTable = (BlockTable)transaction.GetObject(database.BlockTableId, OpenMode.ForRead); 

    // Open the block table record model space for write 
    var blockTableRecord = (BlockTableRecord)transaction.GetObject(blockTable[BlockTableRecord.ModelSpace], OpenMode.ForWrite); 

    blockTableRecord.AppendEntity(dbObject); 
    transaction.AddNewlyCreatedDBObject(dbObject, true); 
    transaction.Commit(); 
  } 
} 

I think this is more readable.

UPDATE: To run special logic, I like the idea of using delegates. I'd refactor the code like this:

private void CreateObjectOnBlockTable(DBObject dbObject) {
  PerformActionOnBlockTable((transaction, blockTableRecord) => {
    blockTableRecord.AppendEntity(dbObject);  
    transaction.AddNewlyCreatedDBObject(dbObject, true);    
  });
}

private void PerformActionOnBlockTable(Action<Transaction, BlockTableRecord> action) {  
  using (var lockedDocument = Application.DocumentManager.MdiActiveDocument.LockDocument())  
  using (var database = Application.DocumentManager.MdiActiveDocument.Database)  
  using (var transaction = database.TransactionManager.StartTransaction()) { 
    // Open the block table for read  
    var blockTable = (BlockTable)transaction.GetObject(database.BlockTableId, OpenMode.ForRead);  

    // Open the block table record model space for write  
    var blockTableRecord = (BlockTableRecord)transaction.GetObject(blockTable[BlockTableRecord.ModelSpace], OpenMode.ForWrite);  

    // Run specific logic
    action(transaction, blockTableRecord);

    transaction.Commit();  
  }  
}  

(the rest of the code would be the same)

PerformActionOnBlockTable can be reused to run arbitrary logic using the transaction and the block table record.

Answer 2

One could argue that both your injection examples are effectively the exact same thing -- sending an interface or a delegate in the PerformAction... method is irrelevant and really a matter of taste. That being said, the separate class implementation for shapes would allow the PerformAction... class to be open for extension but closed for modification. All your shapes would be extensions and your PerformAction... method should never have to change.

Ironically, Shapes are the canonical example of the Open/Closed Principle.