Your comments will be greatly appreciated.

[wizarddrummer]

Greetings,

Your comments and suggestions will be greatly appreciated. I'm posting this here because I would like as many coders as possible to view this post.

I have just inherited the support of an existing system. I will say I am grateful for the opportunity and the income.

When I started looking into the code I was, quite frankly amazed.

Below are four small code segment examples from parts of the program that illustrate what I am up against. There are thousands thousands of lines of code in this system.
(I won't even begin to describe the state of the database design)

What would you do in this situation?

Pray?, Scream?, Cry?, Panic? ...

After looking at the example code what is your opinion of the code you have just looked at?

Thanks, your comments will be extremely helpful to me.

------------------------------ E X A M P L E   O N E   --------------------
.
.
.

ConvertToDollarInLine:
7100 'CONVERT TO DOLLAR STRING
     TempStrA = smsFormatDollarZ(Val(TempStrA), UPT$)
     Return

7500 'Compute an amount in Quotes as ("1"+"2"), out as "3"
     DATECALC% = 0
     PR = 0
     For IntegerX = 1 To Len(TempStrA)
       StringC = Mid$(TempStrA, IntegerX, 1)
       If StringC = "(" Then PR = PR + 1
       If StringC = ")" Then PR = PR - 1
       If PR = 0 Then GoTo 7510
     Next IntegerX
     ReportErrorToLogFile = True
     ErrorText = "Error in calculation at " + FormatedRowAndColumn: Call ShowError(ErrorText): AbortForm = True: GoTo 7599
7510 Cc = IntegerX
     StringC = Left$(TempStrA, Cc): AFTC$(CALCNT) = Mid$(TempStrA, Len(StringC) + 1)
     Total#(CALCNT) = 0: integerFT(CALCNT) = 0
     DURC$(CALCNT) = Mid$(StringC, 2, Len(StringC) - 2)
7530 TempStrA = Left$(DURC$(CALCNT), 1)
     integerLC = InStr(",+-*/^!<>=$@", TempStrA)
     If integerLC = 0 Then GoTo 7535
     DURC$(CALCNT) = Mid$(DURC$(CALCNT), 2)
     If integerLC > 1 Then integerFT(CALCNT) = integerLC - 1
7535 CALC = 1: TempStrA = DURC$(CALCNT): GoSub GETNEXTVARIABLE: DURC$(CALCNT) = RestOfLine$
     If integerLC = 11 Then SVR$ = formVariable$: GoTo 7530
     If formVariable$ = qt + " " + qt Then GoTo 7590
     MaxTextLength = 99: GoSub ReadFormVariable
     vx# = Val(vTransfer)
     If Len(vTransfer) = 8 And Mid$(vTransfer, 3, 1) = "/" And Mid$(vTransfer, 6, 1) = "/" Then
       DATECALC% = 1
       ' !Look! this needs work
       'vx# = CDbl(QfnJulian(vTransfer))
     End If
     wx# = Total#(CALCNT)
     On integerFT(CALCNT) GoTo 7571, 7572, 7573, 7588, 7574, 7575, 7576, 7577, 7578, 7530, 7580
7571   wx# = wx# + vx#: GoTo 7589
7572   wx# = wx# - vx#: GoTo 7589
7573   wx# = wx# * vx#: GoTo 7589
7574   wx# = wx# ^ vx#: GoTo 7589
7575   wx# = (wx# <> vx#) * -1: GoTo 7589
7576   wx# = (wx# < vx#) * -1: GoTo 7589
7577   wx# = (wx# > vx#) * -1: GoTo 7589
7578   wx# = (wx# = vx#) * -1: GoTo 7589
       ' !Look! this needs work
7580   'TD$ = QfnJuliun(CLng(wx#))
       TM% = Val(Mid$(TD$, 1, 2))
       TY% = Val(Mid$(TD$, 7, 2))
       TM% = TM% + vx#
       While TM% > 12: TY% = TY% + 1: TM% = TM% - 12: Wend
       While TY% > 99: TY% = TY% - 100: Wend
       TD$ = smsLeadingOs(TM%, 2) + Mid$(TD$, 3, 4) + smsLeadingOs(TY%, 2)
       ' !Look! this needs work
       'LongTD = QfnJulian(TD$): CD$ = QfnJuliun$(LongTD)
       While CD$ <> TD$
         LongTD = LongTD - 1
         ' !Look! this needs work
         'CD$ = QfnJuliun$(LongTD)
       Wend
       wx# = LongTD: GoTo 7589
7588   If vx# <> 0 Then wx# = wx# / vx#
7589 Total#(CALCNT) = wx#
     ft = 0: GoTo 7530
7590 TempStrA = Str$(Total#(CALCNT))
     If integerFT(CALCNT) = 10 Then TempStrA = smsFormatDollarZ(Total#(CALCNT), UPT$)
     If DATECALC% = 1 Then
       ' !Look! this needs work
       'TempStrA = QfnJuliun(CLng(TOTAL#(CALCNT)))
     End If
     vTransfer = TempStrA: TempStrA = qt + TempStrA + qt + AFTC$(CALCNT)
     formVariable$ = SVR$: GoSub WRITEFORMVARIABLE: SVR$ = ""
7599 Return
.
.
.
-----------------------------  E X A M P L E   T W O   ------------------------------
.
.
.
ladd:
11030 tr = dA + B: GoTo CommonOutVars2:                  '#3  - add
11040 tr = dA - B: GoTo CommonOutVars2:                  '#4  - subtract
11050 tr = dA * B: GoTo CommonOutVars2:                  '#5  - multiply
11060 tr = dA / (B - Var2FlagValue + 1) * Var2FlagValue:                     '#6  - divide
      GoTo CommonOutVars2
11070:                                                   '#7  - get insr co.
11080 Return:                                           '#8  - get warr co.
11090 tr = (dA = B): GoTo CommonOutVars2:                '#9  - set on =
11100 tr = (dA > B): GoTo CommonOutVars2:                '#10 - set on >
11110 tr = (dA < B): GoTo CommonOutVars2:                '#11 - set on <
11120 tr = (dA <> B): GoTo CommonOutVars2:               '#12 - set on <>
11130 tr = (Var2FlagValue And Var3FlagValue): GoTo CommonOutVars2:          '#13 - and
11140 tr = (Var2FlagValue Or Var3FlagValue): GoTo CommonOutVars2:           '#14 - or
11150 tr = ((Var2FlagValue And Var3FlagValue) = 0): GoTo CommonOutVars2:    '#15 - nand
11160 tr = ((Var2FlagValue Or Var3FlagValue) = 0): GoTo CommonOutVars2:     '#16 - nor
11170 tr = (Var2FlagValue = 0): GoTo CommonOutVars2:              '#17 - not
11180 tr = (Var2FlagValue = Var3FlagValue): GoTo CommonOutVars2:            '#18 - xor
11190 tr = ((Var2FlagValue = Var3FlagValue) = 0): GoTo CommonOutVars2:      '#19 - xnor

lRepeat:
11200 ' !look! Repeat command will need some work
      ' IF Var1FlagValue THEN CurrentFormLine = CurrentLineBackUp: RETURN RepeatFormLine:             '#20 - REPEAT
      Return
.
.
.
------------------------------- E X A M P L E   T H R E E   ----------------------
.
.
.
  Select Case FormCommand
     Case 1: GoTo lTitleScreen
     Case 2: GoTo lConvertToDollar
     Case 3: GoTo ladd
     Case 4: GoTo 11040
     Case 5: GoTo 11050
     Case 6: GoTo 11060
     Case 7: GoTo 11070
     Case 8: GoTo 11080
     Case 9: GoTo 11090
    Case 10: GoTo 11100
    Case 11: GoTo 11110
    Case 12: GoTo 11120
.
.
.
-------------------------------- E X A M P L E   F O U R   -------------------------
.
.
.
FactorStyleLease1:
22510 ' FACTOR W.Payment #1 - GECAL,GOLDKEY,SEPAC -
22520 MDEP = Round((AmountFinancedMinusIns + LastTotalInsurance + Admn - Residual) / NormalNumberOfPays, 100)
      MSCHG = Round((AmountFinancedMinusIns + LastTotalInsurance + Admn + Residual) * w.FinanceRate, 100)
      w.Payment = Round(MDEP + MSCHG + AddOn, 100)
      TotalOfPayments = w.Payment * (NormalNumberOfPays - 1)
      GoSub CalcInsurance
      If w.TotalInsPrem > LastTotalInsurance Then LastTotalInsurance = w.TotalInsPrem: GoTo 22520
      GoTo CalcPaymentEnd

FactorStyleLease2:
22610 ' FACTOR W.Payment #2 - VNFSC -
22620 Pay1 = Round((AmountFinancedMinusIns + LastTotalInsurance + Admn - Residual) / NormalNumberOfPays, 100)
      Pay2 = Round((AmountFinancedMinusIns + LastTotalInsurance + Admn) * w.FinanceRate + 0.000000001, 100)
      w.Payment = Round(Pay1 + Pay2, 100)
      TotalOfPayments = w.Payment * (NormalNumberOfPays - 1)
      GoSub CalcInsurance
      If w.TotalInsPrem > LastTotalInsurance Then LastTotalInsurance = w.TotalInsPrem: GoTo 22620
      GoTo CalcPaymentEnd
.
.
.

------------------------------- E N D   E X A M P L E S   ---------------------------

Regards,
wiz

 

[vb5prgrmr]

Hold on ... take a deep breath ... it will be ok ...





Someday




It's not the worst I have seen but ...

Just take it a piece at a time. Don't try to take it all in at once as you could become confused ...


Good Luck

 

[DrJavaJoe]

oh my god. Reverse engineer it, one form or module at a time. Scream, Cry a little, or better yet GOTO newspaper then GOTO Classified then Call Function FIND_NEW_JOB.

Good luck and thanks for posting this, I have bookmarked this thread so when asked why GOTO statements are bad, I can point them to it.

By the way, try to make your subject lines more descriptive.

"Two strings walk into a bar. The first string says to the bartender: 'Bartender, I'll have a beer. u.5n$x5t?*&4ru!2[sACC~ErJ'. The second string says: 'Pardon my friend, he isn't NULL terminated'."

 

[Morningstarr]

Ok granted, that code is SCARY, but remember the old saying "If it ain't broke don't fix it". The application I took over 4 years ago was no where near as bad as what you posted but wasn't good either.

We left working code alone, and concentrated on things that were broken. When time allowed, we fixed small portions of "ugly code" or in your case "Scary code" and anything new that was added, had to follow standards. Fast forward 4 years, we still have "ugly code" but not much.

If you are the decision maker, I would start with a set of standards and work from there.

just my 2 cents

 

[JeffTullin]

You don't need our comments.
The fact that you posted this shows that you already know what you are up against.
This is the kind of code that gave BASIC a bad name.
In your shoes, I'd look for another job -
I consider that code completely unmaintainable.


IF (and that is a BIG if) the program currently does something useful and works most of the time, the tendency will be to take the view 'it works - don't mess with it'.

IF you can debug the code, and you will be called upon to fix problems, then it will be at least 5 times harder to debug and fix than anything that had been written by a good programmer.

Your changes should be procedural (if the compiler/interpreter allows for it), and commented.
If you are stuck with an old compiler, do use meaningful labels for the GOSUBS.

One final note, though:
This is a Visual Basic forum.. whatever that little lot is, it isn't Visual Basic..

 

[DrJavaJoe]

whatever that little lot is, it isn't Visual Basic..
that was my first thought, but as scary as it sounds I think it is.

"Two strings walk into a bar. The first string says to the bartender: 'Bartender, I'll have a beer. u.5n$x5t?*&4ru!2[sACC~ErJ'. The second string says: 'Pardon my friend, he isn't NULL terminated'."

 

[vb5prgrmr]

I agree with you DrJavaJoe that this could be VB code. After the double take I copied and pasted it into the VB IDE and it does seem to be VB code without the procedure headers/footers.

wizarddrummer,

It seems from the snippets that you have provided that you will need to break the labels and gosubs into their own procedures, but this will leave you with a million little prodedures that some may do the same thing as others (redundant code).

So ... Just take it a piece at a time. Don't try to take it all in at once as you could become confused ...

Good Luck

 

[wizarddrummer]

Hi,
WOW thanks for the replys. I have my hands full indeed because part of the program is broken and I am chartered to maintain the code.

What you folks are giving me is ammunition for my case for a complete rewrite new db design and code.

So I appreciate all the comments again.

regards,
wiz

 

[wizarddrummer]

PS
sorry about not having a clearer subject line. I was trying not to lead the comments in any direction by saying something like here's an example of HORRENDOUS code.
I was looking for just what was provided - honest first impression feedback.

regards,
wiz

 

[strongm]

>This is the kind of code that gave BASIC a bad name
>it does seem to be VB code

I have to agree that this code is awful on the surface. But, to be honest, it actually looks like it is a conversion project.

Once upon a time this code was not VB (not version 1,2,3,4,5,6 or .NET)

Some poor bastard has had to convert something from another system. Be happy that you've got a few comments...

 

[DrJavaJoe]

Be happy that you've got a few comments...


' !Look! this needs work



"Two strings walk into a bar. The first string says to the bartender: 'Bartender, I'll have a beer. u.5n$x5t?*&4ru!2[sACC~ErJ'. The second string says: 'Pardon my friend, he isn't NULL terminated'."