Suggestions for better coding?

I have a VBA file that I created, one of my first, that works great. However
I'm not so sure it uses the best coding practices and I'm looking for input
as to how I could improve what I've done, if anyone is willing to give me
some constructive criticism.

Thanks in advance,
Jeff

Portion of working .dvb file:

Private Sub Label_It(startPnt As Variant, endPnt As Variant)

Dim insBlock As AcadBlockReference
Dim attRef As Variant
Dim rotAng As Double
Dim insPnt As Variant
Dim lineLen As Double
Dim util As AcadUtility
Dim pi As Double
Dim I As Integer
Dim below As String
Dim TextLength As Double

pi = 4 * (Atn(1))
rotAng = ThisDrawing.Utility.AngleFromXAxis(startPnt, endPnt)
lineLen = Distance(startPnt, endPnt)
insPnt = endPnt
insPnt(0) = (startPnt(0) + endPnt(0)) / 2#
insPnt(1) = (startPnt(1) + endPnt(1)) / 2#
insPnt(2) = (startPnt(2) + endPnt(2)) / 2#
If rotAng > (pi / 2) + 0.001 And rotAng < (pi * 1.5) + 0.001 Then
rotAng = rotAng + pi
End If
If ThisDrawing.ActiveSpace = acModelSpace Then
Set insBlock = ThisDrawing.ModelSpace.InsertBlock(insPnt, "Mech-38", 1,
1, 1, rotAng)
Else
Set insBlock = ThisDrawing.PaperSpace.InsertBlock(insPnt, "Mech-38", 1,
1, 1, rotAng)
End If
attRef = insBlock.GetAttributes
below = ListBox3.Object
below = Left(below, ((InStr(1, below, "-")) - 2))
If SequenceOption.Value = False Then
below = below & "-" & SeqNum.Value
End If
For I = LBound(attRef) To UBound(attRef)
If attRef(I).TagString = "LINESIZE/MATERIAL" Then
attRef(I).TextString = ListBox1.Object & "-" & ListBox2.Object
ElseIf attRef(I).TagString = "PIPE-IDENTIFICATION-NO" Then
attRef(I).TextString = below
End If
Next I
If LeaderOptYes.Value = True Then
TextLength = GetTextLength(insBlock)
If TextLength > lineLen Then
Dim points(0 To 8) As Double
Dim LeaderObj As AcadLeader
Dim StartPoint As Variant
Dim EndPoint As Variant
Dim NextPoint As Variant
Dim counter As Integer
Dim annoObj As AcadObject
Dim Line As AcadLine

Set annObj = Nothing
StartPoint = insPnt
xxErr = GETXX_FAILED
Do Until xxErr = GETXX_SUCCESS Or xxErr = GETXX_ESCAPE
NextPoint = oGetXX.GetPoint(xxErr, StartPoint, " Next Leader Point:
")
Loop
If xxErr = GETXX_ESCAPE Then
insBlock.Delete
Exit Sub
End If
xxErr = GETXX_FAILED
Do Until xxErr = GETXX_SUCCESS Or xxErr = GETXX_ESCAPE
EndPoint = oGetXX.GetPoint(xxErr, NextPoint, " Leader end point: ")
Loop
If xxErr = GETXX_ESCAPE Then
insBlock.Delete
Exit Sub
End If
For counter = 0 To 2
points(counter) = StartPoint(counter)
Next counter
For counter = 0 To 2
points(counter + 3) = NextPoint(counter)
Next counter
For counter = 0 To 2
points(counter + 6) = EndPoint(counter)
Next counter
If ThisDrawing.Utility.AngleFromXAxis(NextPoint, EndPoint) > (pi / 2) +
0.001 _
And ThisDrawing.Utility.AngleFromXAxis(NextPoint, EndPoint) < (pi *
1.5) + 0.001 Then
rotAng = rotAng + pi
End If
NextPoint = ThisDrawing.Utility.PolarPoint(EndPoint, rotAng, TextLength)
insPoint = ThisDrawing.Utility.PolarPoint(EndPoint, rotAng, (TextLength
/ 2))
If ThisDrawing.ActiveSpace = acModelSpace Then
Set LeaderObj = ThisDrawing.ModelSpace.AddLeader(points, annoObj,
acLineNoArrow)
Set Line = ThisDrawing.ModelSpace.AddLine(EndPoint, NextPoint)
Line.Color = 8
insBlock.InsertionPoint = insPoint
Else
Set LeaderObj = ThisDrawing.PaperSpace.AddLeader(points, annoObj,
acLineNoArrow)
Set Line = ThisDrawing.ModelSpace.AddLine(EndPoint, NextPoint)
Line.Color = 8
insBlock.InsertionPoint = insPoint
End If
End If
End If
End Sub

 

Overall, looks pretty good, though I admit, I'm no code guru, but I've done
my share.

2 things that were told to me that do make sense, though I don't always
follow them:

1. Don't be afraid of long variable names; yeah it means a lot of typing,
but keeps thing clearer down the road

2. Comment your code like mad! Make comments throughout your code that
will be there for your future reference and anyone else that follows.
Should you or someone else look at your code 6 months from now, you won't be
scratching your head trying to remember why you did it that way or what is
was intended to do.

It's good to try to keep it 'clean and organized' too, but that all depends
on the programmer and they read coding.

My $.02...

 

If ThisDrawing.ActiveSpace = acModelSpace Then

You might find this helpful:

Public Function ActiveSpace As AcadBlock
if ThisDrawing.ActiveSpace = acModelSpace then
Set ActiveSpace = ThisDrawing.Modelspace
else
Set ActiveSpace = ThisDrawing.Paperspace
end if
End Function

Then you can insert the block into the active space using
little more than this:

Set insBlock = ActiveSpace.InsertBlock(insPnt....)

 

Jeff,

I agree with Jason, comments are essential. So, I've made some suggestions in the comments of your code below.

Joe
--

Option Explicit

'first, there are too many operations going on inside this routine 'break out some of the functional parts into separate routines

Private Sub Label_It(startPnt As Variant, endPnt As Variant) Dim insBlock As AcadBlockReference Dim attRef As Variant Dim rotAng As Double Dim insPnt As Variant Dim lineLen As Double Dim util As AcadUtility Dim pi As Double Dim I As Integer Dim below As String Dim TextLength As Double

'it's clearer to write this as Atn(1) * 4 pi = 4 * (Atn(1)) rotAng = ThisDrawing.Utility.AngleFromXAxis(startPnt, endPnt) lineLen = Distance(startPnt, endPnt) 'this makes no sense, why set insPnt to a value and then change each element insPnt = endPnt insPnt(0) = (startPnt(0) + endPnt(0)) / 2# insPnt(1) = (startPnt(1) + endPnt(1)) / 2# insPnt(2) = (startPnt(2) + endPnt(2)) / 2#

If rotAng &gt; (pi / 2) + 0.001 And rotAng &lt; (pi * 1.5) + 0.001 Then rotAng = rotAng + pi End If

'since the constant acModelSpace is 1 you could shorten this to ' If ThisDrawing.ActiveSpace Then If ThisDrawing.ActiveSpace = acModelSpace Then &nbsp;&nbsp;Set insBlock = ThisDrawing.ModelSpace.InsertBlock(insPnt, "Mech-38", 1, 1, 1, rotAng)

Else &nbsp;&nbsp;&nbsp;&nbsp;Set insBlock = ThisDrawing.PaperSpace.InsertBlock(insPnt, "Mech-38", 1, 1, 1, rotAng) End If

attRef = insBlock.GetAttributes 'it's unclear what this is doing for you below = ListBox3.Object 'I'm assuming you're parsing a string here below = Left(below, ((InStr(1, below, "-")) - 2))

If SequenceOption.Value = False Then &nbsp;below = below &amp; "-" &amp; SeqNum.Value End If

For I = LBound(attRef) To UBound(attRef) &nbsp;If attRef(I).TagString = "LINESIZE/MATERIAL" Then &nbsp;&nbsp;&nbsp;attRef(I).TextString = ListBox1.Object &amp; "-" &amp; ListBox2.Object &nbsp;&nbsp;ElseIf attRef(I).TagString = "PIPE-IDENTIFICATION-NO" Then &nbsp;&nbsp;&nbsp;&nbsp;attRef(I).TextString = below &nbsp;End If Next I

If LeaderOptYes.Value = True Then &nbsp;&nbsp;TextLength = GetTextLength(insBlock)

If TextLength &gt; lineLen Then &nbsp;&nbsp;&nbsp;&nbsp;Dim points(0 To 8) As Double &nbsp;&nbsp;&nbsp;&nbsp;Dim LeaderObj As AcadLeader &nbsp;&nbsp;&nbsp;&nbsp;'this would be better declared as StartPoint(0 to 2) As Double &nbsp;&nbsp;&nbsp;&nbsp;Dim StartPoint As Variant &nbsp;&nbsp;&nbsp;&nbsp;'this would be better declared as EndPoint(0 to 2) As Double &nbsp;&nbsp;&nbsp;&nbsp;Dim EndPoint As Variant &nbsp;&nbsp;&nbsp;&nbsp;'this would be better declared as NextPoint(0 to 2) As Double &nbsp;&nbsp;&nbsp;&nbsp;Dim NextPoint As Variant &nbsp;&nbsp;&nbsp;&nbsp;'I tend to advocate using Long instead of Integer &nbsp;&nbsp;&nbsp;&nbsp;Dim counter As Integer &nbsp;&nbsp;&nbsp;&nbsp;'always be the most explicit with data types as possible &nbsp;&nbsp;&nbsp;&nbsp;Dim annoObj As AcadObject &nbsp;&nbsp;&nbsp;&nbsp;Dim Line As AcadLine

'this is an undeclared variable which means you're not using &nbsp;&nbsp;&nbsp;&nbsp;'Option Explicit &nbsp;&nbsp;&nbsp;&nbsp;'this is a very bad practice and should never be used! &nbsp;&nbsp;&nbsp;&nbsp;Set annObj = Nothing &nbsp;&nbsp;&nbsp;&nbsp;StartPoint = insPnt &nbsp;&nbsp;&nbsp;&nbsp;xxErr = GETXX_FAILED

Do Until xxErr = GETXX_SUCCESS Or xxErr = GETXX_ESCAPE &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;NextPoint = oGetXX.GetPoint(xxErr, StartPoint, " Next Leader Point:") &nbsp;&nbsp;&nbsp;&nbsp;Loop

If xxErr = GETXX_ESCAPE Then &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;insBlock.Delete &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Exit Sub &nbsp;&nbsp;&nbsp;&nbsp;End If

xxErr = GETXX_FAILED

Do Until xxErr = GETXX_SUCCESS Or xxErr = GETXX_ESCAPE &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;EndPoint = oGetXX.GetPoint(xxErr, NextPoint, " Leader end point: ") &nbsp;&nbsp;&nbsp;&nbsp;Loop

If xxErr = GETXX_ESCAPE Then &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;insBlock.Delete &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Exit Sub &nbsp;&nbsp;&nbsp;&nbsp;End If

For counter = 0 To 2 &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;points(counter) = StartPoint(counter) &nbsp;&nbsp;&nbsp;&nbsp;Next counter

For counter = 0 To 2 &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;points(counter + 3) = NextPoint(counter) &nbsp;&nbsp;&nbsp;&nbsp;Next counter

For counter = 0 To 2 &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;points(counter + 6) = EndPoint(counter) &nbsp;&nbsp;&nbsp;&nbsp;Next counter

If ThisDrawing.Utility.AngleFromXAxis(NextPoint, EndPoint) &gt; (pi / 2) + 0.001 _ &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;And ThisDrawing.Utility.AngleFromXAxis(NextPoint, EndPoint) &lt; (pi * 1.5) + 0.001 Then &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;rotAng = rotAng + pi &nbsp;&nbsp;&nbsp;&nbsp;End If

NextPoint = ThisDrawing.Utility.PolarPoint(EndPoint, rotAng, TextLength) &nbsp;&nbsp;&nbsp;&nbsp;insPoint = ThisDrawing.Utility.PolarPoint(EndPoint, rotAng, (TextLength / 2))

If ThisDrawing.ActiveSpace = acModelSpace Then &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Set LeaderObj = ThisDrawing.ModelSpace.AddLeader(points, annoObj, acLineNoArrow) &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Set Line = ThisDrawing.ModelSpace.AddLine(EndPoint, NextPoint) &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Line.color = 8 &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;insBlock.InsertionPoint = insPoint

Else &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Set LeaderObj = ThisDrawing.PaperSpace.AddLeader(points, annoObj, acLineNoArrow) &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;'I think this was suppose to be PaperSpace? &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Set Line = ThisDrawing.ModelSpace.AddLine(EndPoint, NextPoint) &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Line.color = 8 &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;insBlock.InsertionPoint = insPoint &nbsp;&nbsp;&nbsp;&nbsp;End If &nbsp;&nbsp;End If End If End Sub

 

Jason,
Thanks for the comments!
As for commenting my code, that's a bad habit I've always had (in my usual
work as well as coding) and I'm trying desperately to change my ways.

Jeff

 

on the comment of "too many operations inside one routine" what he is
saying.
If you put say Function Add_Tags(dblVar1 as double, ect...) then you pass
the variables from the calling function/sub this way if you add more code to
the program then you would not need to retype allot of code(recycle).
On the getXX as var. I use the get function as well and declaring them as
doubles. The reason you should try and avoid Variants is that it take's up
computer memory and will slow the program down if they are declared
(string,int,byte ect..) the program will allocate less memory space for that
variable.

hope this helps,
Andy

 

Tony,
Thanks for that little tidbit! On testing it I did realize that I wasn't
accounting for the case of the user working in modelspace through a
Pviewport. I just changed one line of your code to accomodate that:

If ThisDrawing.ActiveSpace = acModelSpace OR ThisDrawing.Mspace = True Then

and all is well.
Thanks again,
Jeff

 

and that works for you?
this works for me:
Dim ip As Variant
ip = adoc.Utility.GetPoint(, "Pick point for block insertion: ")

this gives compile error "Can't assign to array"
Dim ip(2) As Double

ip = adoc.Utility.GetPoint(, "Pick point for block insertion: ")

how are you overcoming the fact that vb can't assign directly to an array?

 

Here is the actual code from my project. (wrote in vb.net)

InstantiateAutoCAD()

GetUserCords()

Me.CabinetXpos = P(0)

Me.CabinetYpos = P(1)

Select Case frmCabinet.cmbCabType.SelectedIndex

Case 0, 2

Me.CabinetZpos = 0

Case Else

'Me.CabinetZpos = P(2)

Me.CabinetZpos = InputBox("Upper Elevation", "Upper Elevation",
Me.CabinetZpos)

End Select



End Sub

Sub New()

End Sub

Private Function InstantiateAutoCAD()

Try : A2K = GetObject(, "AutoCAD.Application")

Catch : A2K = CreateObject("AutoCad.Application")

End Try

A2K.WindowState = AutoCAD.AcWindowState.acMax

A2K.Visible = True

A2Kdwg = A2K.ActiveDocument

End Function

Private Function GetUserCords()

Dim A2K As AutoCAD.AcadApplication

Dim A2Kdwg As AutoCAD.AcadDocument

Try

A2K = GetObject(, "AutoCAD.Application")

Catch

A2K = CreateObject("AutoCAD.Application")

End Try

A2K.Visible = True

A2K.WindowState = AutoCAD.AcWindowState.acMax

A2Kdwg = A2K.Application.ActiveDocument

A2Kdwg.Utility.InitializeUserInput(1, "Boo")

Dim strMessage As String = "Swith to AutoCAD and "

Select Case frmCabinet.cmbCabType.Text

Case "Upper"

strMessage += "select the bottom front left corner of the upper cabinet."

Case Else

strMessage += "select the bottom front left corner of the " &
frmCabinet.cmbCabType.Text & " cabinet." & vbCrLf & _

"NOTE: the ToeKick height will be added to your placement."

End Select

MessageBox.Show(strMessage, "Select Start postion", MessageBoxButtons.OK,
MessageBoxIcon.Information)

frmCabinet.Hide()

P = A2Kdwg.Utility.GetPoint(, "Select start Point")

End Function

Thanks,

Andy Elmore

 

Andrew,

Your "Dim P" or "Public P" statement (which I've heard can lead to arrest)
must be feeling shy, because he wasn't anywhere in your post...
Please show us where P is defined.

Thank you,

James

Apologies for the groaner... it's been a long day... I'm tired.

 

#Region "Shared"

Shared WallEnd As String

Shared dsDataSetMaterial As DataSet

Shared dblBoxHeight As Double

Shared dblBoxDepth As Double

Shared dblDadoDepth As Double = 7

Const vtLaminateThickness As Decimal = 0.8

Public A2K As AutoCAD.AcadApplication

Public A2Kdwg As AutoCAD.AcadDocument

Public P As Object

Shared blnDrawers As Boolean

#End Region

 

Mark-

The problem is that you are dimensioning the array. AutoCAD needs an
undimensioned array to populate just as if you were using a VB command
like Split. Use this:

Dim ip() As Double
ip = adoc.Utility.GetPoint(, "Pick point for block insertion: ")

___________________________
Mike Tuersley
AutoCAD Clinic
Rand IMAGINiT Technologies