Help clean up code

[esskaykay]

I have some VB code I modified to add functionality to an older CAD routine (originally written by a consultant). The new code works fine and appears to do what I want. My question is, not being a programmer, I’m sure I did this quite primitively. I was wondering if anyone could peruse my code to see if there are any suggestions as to cleaning it up a bit (particularly with the numerous IF statements.

CODE:

If txtOrig_Mtrl = "" Then

If Not txtSize_2.Text = "" And Not IsNull(txtSize_2.Text) And Not txtSize_2.Text = 0 Then
If txtConst_Date = "9999" Then

sStoPipeText = Trim(txtLength_1.Text) + "'-" + Trim(txtSize_1.Text) + Chr(34) + "x" + Trim(txtSize_2.Text) + Chr(34) + " " + Trim(cboMaterial.Text) + " (*)"

Else

sStoPipeText = Trim(txtLength_1.Text) + "'-" + Trim(txtSize_1.Text) + Chr(34) + "x" + Trim(txtSize_2.Text) + Chr(34) + " " + Trim(cboMaterial.Text) + " (" + Trim(txtConst_Date.Text) + ")"

End If

If Not sLength = txtLength_1.Text Or Not sSize = txtSize_1.Text Or Not sSize2 = txtSize_2.Text Or Not sDate = txtConst_Date.Text Or Not sMaterial = cboMaterial.Text Then

ScanDrawingRemoveSTOText
StoPipeTextLine
End If
Else
If txtConst_Date = "9999" Then

sStoPipeText = Trim(txtLength_1.Text) + "'-" + Trim(txtSize_1.Text) + Chr(34) + " " + Trim(cboMaterial.Text) + " (*)"

Else
sStoPipeText = Trim(txtLength_1.Text) + "'-" + Trim(txtSize_1.Text) + Chr(34) + " " + Trim(cboMaterial.Text) + " (" + Trim(txtConst_Date.Text) + ")"

End If
If Not sLength = txtLength_1.Text Or Not sSize = txtSize_1.Text Or Not sSize2 = txtSize_2.Text Or Not sDate = txtConst_Date.Text Or Not sMaterial = cboMaterial.Text Then

ActiveModelReference.UnselectAllElements
ActiveModelReference.SelectElement ele
ScanDrawingRemoveSTOText
StoPipeTextLine
End If
End If

Else
If Not txtSize_2.Text = "" And Not IsNull(txtSize_2.Text) And Not txtSize_2.Text = 0 Then
If txtConst_Date = "9999" Then

sStoPipeText = Trim(txtLength_1.Text) + "'-" + Trim(txtSize_1.Text) + Chr(34) + "x" + Trim(txtSize_2.Text) + Chr(34) + " " + Trim(cboMaterial.Text) + "-" + Trim(txtOrig_Mtrl.Text) + " (*)"

Else
sStoPipeText = Trim(txtLength_1.Text) + "'-" + Trim(txtSize_1.Text) + Chr(34) + "x" + Trim(txtSize_2.Text) + Chr(34) + " " + Trim(cboMaterial.Text) + "-" + Trim(txtOrig_Mtrl.Text) + " (" + Trim(txtConst_Date.Text) + ")"

End If
If Not sLength = txtLength_1.Text Or Not sSize = txtSize_1.Text Or Not sSize2 = txtSize_2.Text Or Not sDate = txtConst_Date.Text Or Not sMaterial = cboMaterial.Text Then

ScanDrawingRemoveSTOText
StoPipeTextLine
End If
Else
If txtConst_Date = "9999" Then

sStoPipeText = Trim(txtLength_1.Text) + "'-" + Trim(txtSize_1.Text) + Chr(34) + " " + Trim(cboMaterial.Text) + "-" + Trim(txtOrig_Mtrl.Text) + " (*)"

Else
sStoPipeText = Trim(txtLength_1.Text) + "'-" + Trim(txtSize_1.Text) + Chr(34) + " " + Trim(cboMaterial.Text) + "-" + Trim(txtOrig_Mtrl.Text) + " (" + Trim(txtConst_Date.Text) + ")"

End If
If Not sLength = txtLength_1.Text Or Not sSize = txtSize_1.Text Or Not sSize2 = txtSize_2.Text Or Not sDate = txtConst_Date.Text Or Not sMaterial = cboMaterial.Text Then

ActiveModelReference.UnselectAllElements
ActiveModelReference.SelectElement ele
ScanDrawingRemoveSTOText
StoPipeTextLine
End If
End If
End If


If this appears too daunting, I understand.
Thanks,
SKK

 

[tehNellie]

Indenting your code so that it's readable (code tags) would help, some context as to what it is actually doing would also help you. Comments in meaningful places in your code also help not only you, but fools like me sat scratching their heads trying to figure out what it is supposed to be doing.

As you've not included any variable declaration the first thing I'd suggest you do is ensure that you are using OPTION EXPLICIT which, amongst other things forces you to use strongly typed variables

Some of your sStoPipeText = "huge amounts of stuff" are repeated in at least two places.

I presume:
ScanDrawingRemoveSTOText
StoPipeTextLine

Are functions/procedures? Maybe they use those strings that you've been building? Who knows

You might consider using something like

'This is used to do <somethingn or other>
strA_Meaningful_Name = "huge amounts of stuff"

'this is the alternate version when certain conditions aren't met and does stuff
strA_meaningful_name2 = "Different huges amounts of stuff"


if Replace(Nz(txtsize_2.Text, ""), "", 0) <> 0 then
  'The textbox isnt empty or 0 so setting whatever this does to this value
  sstopipetext = strA_meaningful_name
else
  'it's a zero/blank value so doing something different instead.
  sstopipetext = strA_meaningful_name2
end if

At least then you're only building a huge, unreadable, string in one place at the start of your code and then assigning a much shorter variable, with a name that makes sense in the context of what you're doing further on down the line.

It does look like you've got a lot of duplication going on both in form and in function, but without some context as to what this block of code is supposed to be doing it's very difficult to make any suggestion. You go to great lengths to build these variables and then never actually use them in the code.

 

[esskaykay]

Thank you. I will try cleaning as you noted. However, like I said, this was originally created by someone else. All I did was add the additional IF. I'm going to try creating separate code and calling it. I'm just a bit confused as how to request a specific parameter. Let me play with this a bit. I'll keep you posted.

Thanks again,
SKK

 

[tehNellie]

Before you start coding, comment what you have. What are you doing in each step and why? Even if you think you're clear in your mind what is going on, write it down

what does:

sStoPipeText = Trim(txtLength_1.Text) + "'-" + Trim(txtSize_1.Text) + Chr(34) + "x" + Trim(txtSize_2.Text) + Chr(34) + " " + Trim(cboMaterial.Text) + "-" + Trim(txtOrig_Mtrl.Text) + " (" + Trim(txtConst_Date.Text) + ")"

mean in comparison to the other variants of the string? Will you remember when you look at it again in 6 months time.

If txtConst_Date = "9999" Then

implies that you might be expecting a date but you're testing for "9999" why?

If it doesn't help you, it will certainly help whoever has to go through it next