Re-factoring Inherited Code

[fredlo2010]

Hey everyone,

I have a question for all the VBA professionals out there. As part of my new job I inherited a lot of code. The code is fairly complicated and it contains a lot of googled pieces stitched together. Nothing wrong with that I have done it too. But now I have to understand that code and debug it and implement new solutions. But its so messy that I cannot understand what the code is doing sometimes, unless I re-factor it and break it down into pieces.

Sometimes the "IF" statements are so big that I have to scroll several times to see the "ELSE" Or there is only one procedure that does about a dozen things. Loops to find the last row...etc

But then; if I put it back to the way it was; I will have to do it all over again, and this is just to debug or make a small update. Now this will bring problems:
1- The regular problems that come along with re-factoring: uncovering bugs, messing up something somewhere else...
2- The people that coded this might look at me as a pretentious guy that's just doing things the way he wants.

I am wondering if this is just me or this something you have experienced something similar. And how to bring it up to my boss or something.


Simply the code is too hard to understand without re-factoring.

Thanks in advance for the answers :)

PS: I found this website that talks a lot about re-factoring http://sourcemaking.com/refactoring

[fredlo2010]

and yeah from the website I just added this came along ( Last two paragraphs) http://sourcemaking.com/refactoring/why-should-you-refactor


This understandability works another way, too. I use refactoring to help me understand unfamiliar code. When I look at unfamiliar code, I have to try to understand what it does. I look at a couple of lines and say to myself, oh yes, that's what this bit of code is doing. With refactoring I don't stop at the mental note. I actually change the code to better reflect my understanding, and then I test that understanding by rerunning the code to see if it still works.

Early on I do refactoring like this on little details. As the code gets clearer, I find I can see things about the design that I could not see before. Had I not changed the code, I probably never would have seen these things, because I'm just not clever enough to visualize all this in my head. Ralph Johnson describes these early refactorings as wiping the dirt off a window so you can see beyond. When I'm studying code I find refactoring leads me to higher levels of understanding that otherwise I would miss.

[Bob Phillips]

That is not what I understand by refactoring. To me, refactoring is going over code that you understand but which is not efficient/not clear/not easily maintainable, and making it so. If you don't know how the code works/does, how can you properly change anything and be sure you haven't messed up?

And again, to quote another site on refactoring ... If you have a poorly factored program that does what the customer wants and has no serious bugs, for the love of Mike leave it alone! When you need to fix a bug or add a feature, you RefactorMercilessly the code that you encounter in your efforts. Thus, RefactorMercilessly can live in harmony with IfItIsWorkingDontChange. -- (I have to admit I sometimes fail to heed those words :))

But, defensive office politics approach. Tell your boss the code is not good, needs serious work to make more maintainable, and needs a piece of work just to identify what needs to be done. He will of course ask do wehave an immediate problem, you say no, so he declines your kind offer. And when the proverbial hits the fan, you say that '... back in blah blah, I suggested we tackled this'.

[fredlo2010]

Thanks for the reply xld.

The key word here is as told by management "Get familiar with the code" but its so hard to read that I cannot get familiar with it. Sometimes I spend minutes and minutes reading the code and after rereading and stepping through it then i will go "Ah! this is whats doing!" (very funny indeed) I guess what I don't want is for them to expect that "Getting familiar with the code" means I will be able to debug something right away or implement something really fast.

Another thing is that they want to improve the code every time the something breaks. If the Code fails to select a sheet because it was hidden; oh we need to check for hidden sheets before selecting them which is not the real problem here. So I will end up with a crazy madness. lol The expectations, I see, is to build a skyscraper based on a shake foundation (the disaster is coming)

And yes if its not broken don't fix it but what If i have to add more stuff.?

Yes for now I am doing what you suggest. I am making them aware of serious issues and that might not be the best solution. And yeah the program is not fine it will break almost every day something different for different users and it takes me forever to figure things out. LOL

Thanks xld. :)

[SamT]

First: what can you do once, that might be applicable to many VBA projects?
Column Numbers for one, I hate to have to think about which column is Columns(7). Create an Enumerations modul and enumerate the column numbers at least to 26, name mthe colA, colB, colC, etc. (the uppercase column letter really jumps out that way.) When you run into a column above Z, add it to the Enum "ColumnLetters." If you store this in Personal.xls and put Option Private Module at the top of the page, you can copy it into any VBA Project, (just remove the Option in the copy.) As you refactor a procedure, replace all magic number column references with their enumerated constants.

LastRow is a very common line of code. There is a very good and comprehensive LastUsedCell procedure in our KB. Modify it slightly to handle any request for a last used or next used whatever.
Function lastUsed(WkBk As Workbook, Sht As Sheet, TypeLast As String, Optional, Rw As Long = 0, Optional Col As Long = 0) As Variant
Select Case TypeLast
Case Last
if Rw + Col = 0 Then Error
If Rw <0 or col < 0 or (rw >0 and col > 0) then error
If Col > 0 then Last Used = LastCol function
else LastUsed = lastrow function As Long
Case "Next"
Same as above but add 1 As Long
Case Cell
Last used cell on sheet As Range
End Select
Do this for every commonly used procedure, then paste the modules into the VBA Project you are refactoring.

Second, Follow Xld's advice inre IfItIsWorkingDontChange and RefactorMercilessly the code that you encounter in your efforts.

Ctrl+h is a big help in preliminary refactoring a procedure. If you see a variable used as a column number, use Ctrl+h to replace all instances of it in the Procedure with a constant from the ColumnLetters enumeration. If you see a Cells(r, c), Unless the c is part of a loop, replace the c number with the Enumerated Constant. Any time you think of a better name for a variable, Ctrl+h it.

All rules are made to be broken... On database style workshhets, you have a lot of calls for the next empty cell in Column A. I will write a one line function named NextRow() as Long and copy it into every Database style Sheet's code page. Then I will call it from some sub with X = Sheets("DBSheetName").NextRow or
With Sheets("DBSheetName)
X = .nextRow
End with. I only do this with one-line functions. It's also handy with Function GetRecord(RecNum As long) As Variant

IM(not so)HO, The OK_Click sub here (http://www.vbaexpress.com/forum/showthread.php?50522-If-statement-with-CommandButton&p=314538&viewfull=1#post314538) is an example of well factored code. Never mind the error in a sub below (Missing "Next.") The exceptions are the Label Click and Double Click subs, which can be refactored to all 80 of them calling one of two BackColorMe subs and one SetMyTag sub with a 40 line Case Statement
Sub Label1_Click()
BackColorMe1("Label1")
SetMyTag("Label1")
End Sub

SetMyTag(LabelName As String)
Case "Label1"
If Me.Controls(LabelName).BackColor = Red, Then Me.Controls(LabelName).Tag = "A1:A2"And, yes. I personally would write it that way for readability vice using a "With Me.Controls(LabelName)" which takes three lines per Case. Speed in this usage is not a factor.

In closing: When you have to work o a procedure, make the variable mean something, re-study the code, make the code better. Move as much of the code as possible to another sub or function. Move the sub-subs and functions to modules with meaningful names that group the subs and functions by usage.

Always remember that Perfect is the enemy of Good.

[SamT]

I refactored the Label Event code in the code linked to above to eliminate a bunch of code that did the same things for each control. it's in post #42 (http://www.vbaexpress.com/forum/showthread.php?50522-If-statement-with-CommandButton&p=314629&viewfull=1#post314629)

[fredlo2010]

Thanks a lot for the tips and help SamT.

The re-factoring part itself is not that hard unless you come across code like the one bellow that re really scratch your head. The code is supposed to find the column letters and adjust them to a formula.:


For j = 0 To 9 Select Case j
Case 0
alpha = -MANUALS
Case 1, 6
alpha = -1
Case 2, 5
alpha = 1
Case 3
alpha = 3
Case 4
alpha = 4
Case 7
alpha = -2
Case 8
alpha = 5
Case 9
alpha = 6

End Select

If (col2 + alpha) > 26 And (col2 + alpha) <= 52 Then
col3(j) = "A" & chr(col2 + 64 - 26 + alpha)
Else
If (col2 + alpha) > 52 And (col2 + alpha) <= 78 Then
col3(j) = "B" & chr(col2 + 64 - 52 + alpha)
Else
If (col2 + alpha) > 78 And (col2 + alpha) <= 104 Then
col3(j) = "C" & chr(col2 + 64 - 78 + alpha)
Else
col3(j) = chr(col2 + 64 + alpha)
End If
End If
End If
Next

Which can be achieved with something like ( I changed the whole thing to R1C1 formulas)

Split(Cells(1,Col2).Address(true,false),"$")(0)


Or this one to create a drop-down menu of unique values


Sub CreateDropDownMenu()

Dim rowCounter As String
Dim dupe As String
Dim CellRange As Range








Range("A:A").Select
Selection.Copy
Range("AC:AC").Select
ActiveSheet.Paste
Application.CutCopyMode = False


rowCounter = 6


Do Until IsEmpty(Cells(rowCounter, 1))
Cells(rowCounter, 1).Select
rowCounter = rowCounter + 1
Loop


rowCounter = rowCounter - 1




Set CellRange = Range(Cells(6, 29), Cells(rowCounter, 29))
CellRange.Select




Selection.Sort Key1:=Range("AC6"), Order1:=xlAscending, Header:=xlNo, _
OrderCustom:=1, MatchCase:=False, Orientation:=xlTopToBottom, _
DataOption1:=xlSortNormal

Cells(6, 30).Select
ActiveCell.Formula = "=IF(AC6=AC7,1,0)"


Cells(6, 30).Select
Selection.Copy
Set CellRange = Range(Cells(6, 30), Cells(rowCounter, 30))
CellRange.Select
ActiveSheet.Paste
Application.CutCopyMode = False


Selection.Copy
Selection.PasteSpecial Paste:=xlValues, Operation:=xlNone, SkipBlanks:= _
False, Transpose:=False
Application.CutCopyMode = False




Range(Cells(6, 29), Cells(rowCounter, 30)).Select

Selection.Sort Key1:=Range("AD6"), Order1:=xlAscending, Key2:=Range("AC6") _
, Order2:=xlAscending, Header:=xlNo, OrderCustom:=1, MatchCase:= _
False, Orientation:=xlTopToBottom, DataOption1:=xlSortNormal, DataOption2 _
:=xlSortNormal




rowCounter = 6
Cells(6, 28).Select


Do Until IsEmpty(Cells(rowCounter, 29))


dupe = Cells(rowCounter, 30).value

If dupe = 0 Then
rowCounter = rowCounter + 1
ActiveCell.Offset(1, 0).Select
Else
Range(Cells(rowCounter, 29), Cells(rowCounter, 30)).Select
Selection.Delete Shift:=xlUp
End If


Loop


Range("AD:AD").Select
Selection.Clear


With Selection.Interior
.ColorIndex = 2
.Pattern = xlSolid
End With
Range("A5").Select


Cells(5, 1).Select


End Sub

which became this :

Sub CreateDropDownMenu()


Dim iLastRow As Integer
Dim iFirstRow As Integer
Dim rSource As Range


iLastRow = Cells(Rows.Count, 1).End(xlUp).row
iFirstRow = Cells(1, 1).End(xlDown).row

Set rSource = Cells(iFirstRow, 1).Resize(iLastRow, 1)
' Autofilter Unique values

rSource.AdvancedFilter xlFilterCopy, rSource, Range("AC5"), True

' Sort the dropdown values
Range("AC5").Sort Range("AC6"), Order1:=xlAscending, Header:=xlYes
' Select Landing
Range("A5").Select


End Sub

Yeah sometimes I have to hold myself from using "With"...

Thanks a lot for the help :)

[SamT]

there's nothing wrong, per se, with With. In fact by the time I had refactored the Tag value assignment 4 times I wound up with a 40 case (one line per) Select Case inside a If..Then...Else inside a With. But it's beautiful now.

Since you're now making BIG MONEY as a full time programmer. invest $80 and buy UltraEdit. (http://www.ultraedit.com/) Using Excel's Fill and Series functions and UE's Regular expressions, I wrote all 80 of those Label subs in less than 10 minutes. Then I refactored the 40 case Select Case statement 4(5?) times in another hour, including error correction and thought time.

BTW, I'm only using UE32 (version8) from 2001.

[Bob Phillips]

Since you're now making BIG MONEY as a full time programmer. invest $80 and buy UltraEdit. (http://www.ultraedit.com/)

You can get Notepad++ (http://notepad-plus-plus.org/), which is just as good in my view, for free, with free updates.

[fredlo2010]

Yeah I use Notepad++ at work to edit some if the code and see some hidden variables. I have been looking for an addin for VBA code to show the code formatted as VBA; have not found it yet. :) and for compare I use Tortoise SVN. :)

Thanks for the suggestions.

:)

[Bob Phillips]

It has a VB plugin, it's the same.

[SamT]

UE shows VBA code (and many others) in VBE colors. You can also customize the colors better than you can in the VBE.

[Bob Phillips]

It has a VB plugin, it's the same.

And you could write your own if you don't like the provided plugins, it's not hard, I did one for Excel formulas.

[fredlo2010]

Yes, I know. Someone here wrote one for sql I think.

I was looking at it yesterday ... It will require some time to do something like this though.

Thanks

[Bob Phillips]

I did say you could write your own, but why bother, what is wrong with the VB plugin?