Do's and Don'ts So Others Won't

I'm sure that you pros have many don'ts when it comes to VBA programming. A good example is the use of GoTo Labels (beaten to death I'm sure). If you have the time and you think of a good don't, please post it here.
I would like to start off by asking about Option Explicit. I understand what it does ie forcing the programmer to define variables but is it necessary? I see some use it when they help others with code and some don't use it. When don't you and when do you use it?

ALWAYS use it. (set it in options)

See also this thread.

Use a tool (e.g., SmartIndenter) to indent your code. It is much easier to understand, and many compile errors can be avoided just by appearance.

@romperstomper, good link, thank you.
@shg, where is something like SmartIndenter? I looked for it in 'Tools' but didn't see it.

You can get it here.
Code Cleaner is another must.

about Option Explicit.

People who don't use it aren't programming, they're just playing with their computer.

Code Cleaner is another must.

Ditto that. I used to do manually what it automates; I cried when I found it. Not like I cried when I discovered VisiCalc, but a good cry nonetheless.

I would add this, high on the list: adopt a variable naming convention and never deviate from it, unless you find one you like better.

Originally Posted by shg

I would add this, high on the list: adopt a variable naming convention and never deviate from it, unless you find one you like better.

Agreed. In fact consistency generally in your programming will stand you in very good stead. (unless of course you are trying to remain anonymous on an internet forum: it took one member here 3 posts to work out who I am. )

Originally Posted by romperstomper

(unless of course you are trying to remain anonymous on an internet forum: it took one member here 3 posts to work out who I am. )

I think I can probably guess who that might have been

I suspect it was Richard Schollar... the same point holds true for formula bods also... everybody has a style to some extent (good/bad).

People who don't use it aren't programming, they're just playing with their computer.

Too funny, I've mostly just been playing with my computer. I'm going to *** through my projects thus far and use Option Explicit. Once I am done, will I be an official programmer?

I'm also going to look into the Code Cleaner you folks are talking about.

You guys are awesome.

I just installed and ran the Code Cleaner on one my projects but what did it actually do? Everything looks the same.

Originally Posted by Mordred

I just installed and ran the Code Cleaner on one my projects but what did it actually do? Everything looks the same.

Everything should look the same (but maybe a little more sparkly). It cleans out the p-code garbage that builds up as you repeatedly edit VBProjects.

Oh, and always remember that you virtually never have to select cells in VBA

I'm still trying to break the habit of selecting cells and ranges and what-not but my fingers think for them selves sometimes (cursed things).

SmartIndenter is sweet!

hi Modred,

There's an overlap with what's already been offered, but here is a link to a list of some tools which I find handy: http://www.excelforum.com/2078299-post14.html

Plus another freeware comparision tool that I've found quite helpful for comparing versions of VBA code (just being a self-taught chap, with no specific "version control app" which I've seen some IT guys use for other languages): http://www.formulasoft.com/vba-code-compare.html

hth
Rob

Never write a line of code like this:

If Replace(Join(Application.Transpose(Application.Transpose(Cells(j, 1).Resize(, 7))), ""), Cells(j, 4), "") = Replace(Join(Application.Transpose(Application.Transpose(Cells(j + 1, 1).Resize(, 7))), ""), Cells(j + 1, 4), "") Then

As if that could happen...

Re Option Explicit, I always use it, just never quote it in my posts.

I don't use any VBA tools but keep meaning to look at them as I gather there are some very useful ones.

Do learn about passing values to and from arrays (sooner than me anyway, they still confuddle me).

Don't keep a bottle of lighter fluid in the booze cupboard

Dom

It was indeed Richard. Unnerved me a bit.

Yeah, it is very hard to hide from your style...

I also guessed a couple of members correctly....who had different names in other forums...

I assumed my whack it really hard with a hammer until it gives in style would have been transparent so saw little point in using a different name.

Have we gone a little off topic here maybe?

Dom

Originally Posted by Domski

Have we gone a little off topic here maybe?

Dom

that tends to happen in many watercooler threads.... just like in real world watercooler conversations.... someone else comes into the lunch room and inserts different thoughts based on what they heard when they walked in...

Some of the tangents that the Lounge at Mr Excel used to go off on were interesting to say the least, usually thanks to a certain Greg Truby.

Dom

But you have to admire his high-falutin' language.

Alright, pop-quiz hot shots. Do you re-use variables? If you have multiple (non-nested) loops, do you re-use i for integer?
What if you use a string to find a workbook name using the application.getopenfilename dialog box. Would you redefine the string after opening the workbook to get rid of the path in the name?

Examples:

Public Sub Example()

Dim i As Integer
Dim strFileName As String

For i = 1 To 4
    Debug.Print i
Next i

For i = 8 To 4 Step -1
    Debug.Print i
Next i

strFileName = Application.GetOpenFilename(Title:="Please Choose a file to Open.")
Workbooks.Open strFileName
strFileName = ActiveWorkbook.Name

End Sub

Personally, I wouldn't re-use a variable even in separate loops. If I ever needed to use multiple loops in code they would be doing different things, so I'd differentiate them to avoid any confusion. If this was 40 years ago and memory allocation was more of an issue, perhaps.

I reuse scratch variables if their scope of use is within a few lines of code, and I identify them as such in the declaration (I comment all my variable declarations as to purpose when I'm not just playing with my computer).

For variables that index arrays, I name them consistent with the array; iChr indexes asChr, which has nChr elements.

I wouldn't reuse your string variable below because in the first case it's a path and filename, and in the second, just the filename.

Originally Posted by davegugg

Alright, pop-quiz hot shots. Do you re-use variables? If you have multiple (non-nested) loops, do you re-use i for integer?
What if you use a string to find a workbook name using the application.getopenfilename dialog box. Would you redefine the string after opening the workbook to get rid of the path in the name?

I still use J as a loop variable where the loop is just a counter, if I actually do anything with the loop variable a more meaningful name is appropriate!!

If you just wanted the total of hours, add column D as a helper to your first sheet :-

=Month(a1)

then you can total for each month with :-

=SUMIF(Sheet1!$D$2:$D$13,9,Sheet1!$C$2:$C$13)-SUMIF(Sheet1!$D$2:$D$13,9,Sheet1!$B$2:$B$13)

Pivot table is the best way to go!!!!!!

Oh one other one:
If you are not using the Call keyword, or returning a value from a Function, do not enclose arguments in parentheses.

Originally Posted by romperstomper

If you are not using the Call keyword, or returning a value from a Function, do not enclose arguments in parentheses.

Note the bold part.

do not enclose arguments in parentheses

Why? Is that a style preference? I come to actually like parentheses around arguments as it (in my mind) lets me know exactly where an argument is being passed.
@pb71, I'm one for asking questions. I like to know the whys because I think they are necessary if you want to do well with whatever task at hand. Knowing how isn't enough for me.

No it's syntax not style. Try it with an object variable.
For example:

Sub StopUsingParentheses()
    Dim rngTest As Range
    Dim x As Long, y As Long
    
    Set rngTest = Range("A1")
    ' oops!
    macro1 (rngTest)
    
    x = 4
    y = 5
    
    ' this won't even compile (so commented out)
    ' macro2 (x, y)
    
    ' and this just seems weird:
    macro2 (x), (y)
    
    ' but you can do:
    Call macro1(rngTest)
    Call macro2(x, y)
    
End Sub
Sub macro1(rng As Range)
    MsgBox rng.Address
End Sub
Sub macro2(var1, var2)
    MsgBox "Var 1 is: " & var1 & "; var2 is: " & var2
End Sub

You'll have to parden my naivety romperstomper, but you are using parentheses in your last example and so I am a little confused. Specifically where it works ie:

Call macro1(rngTest)
Call macro2(x, y)

Could you elaborate a little more regarding this?

I just wanted to thank you guys, especially Andrew, who has steered me regarding using .Select. I took this code,

Sub IPIupdate()
    Application.ScreenUpdating = False
    Dim CanName As String, fName As String
    Dim numCount As Long, numCount2 As Long, mthCount As Integer, iCount2 As Long
    Dim rngName As Range, dtRng As Range, cpyRng1 As Range, cpyRng2 As Range
    mthCount = 12
    Do Until fName <> ""
        fName = InputBox("Enter the source file name", "Source File Name")
    Loop
    OpenFile (fName)
    Windows("IndustryPriceIndex.xls").Activate
    Application.Worksheets("Mthly").Select
    For Each iCount In Range("Cansim")
        numCount = 0
        CanName = iCount.Value
        Windows(fName).Activate
        For iCount2 = 1 To Sheets.Count
            With Sheets(iCount2)
                Sheets(iCount2).Select
                Range("A1:Q80").Select
                Set rngName = Cells.Find(what:=iCount)
                If Not rngName Is Nothing Then
                    rngName.Select
                    ActiveCell.Offset(1, 0).End(xlToRight).End(xlDown).Select
                    Do While ActiveCell = ".."
                        ActiveCell.Offset(0, -1).Select
                        numCount = numCount + 1
                    Loop
                    numCount2 = mthCount - numCount
                    ActiveCell.Offset(0, -numCount2).Resize(, numCount2 + 1).Select
                    Set cpyRng1 = Selection
                    Windows("IndustryPriceIndex.xls").Activate
                    iCount.End(xlToRight).Select
                    ActiveCell.Offset(0, -numCount2 + 1).Resize(, numCount2 + 1).Select
                    Set cpyRng2 = Selection
                    cpyRng2.Cells.Value = cpyRng1.Cells.Value
                End If
                If Not rngName Is Nothing Then Exit For
            End With
        Next iCount2
    Next iCount
    Application.ScreenUpdating = True
End Sub

and changed it to this code,

Option Explicit
Sub IndPrcIndex()
    Application.ScreenUpdating = False
    Dim fName As String
    Dim numCount1 As Integer, iCount As Variant, dataFind As Variant
    Dim rngName As Range
    Dim dFind1 As Range, dFind2 As Range, dSelect1 As Range, cell As Range

    Do Until fName <> ""   ''''Prompt user for the name of the source file to update from''''
        fName = InputBox("Enter the source file name", "Source File Name")
    Loop
    OpenFile fName  ''''Open the file''''
    For Each cell In Application.Workbooks("IndustryPriceIndex.xls").Worksheets("Mthly").Range("Cansim")    ''''Loop through each cell in Range("Cansim")''''
        For iCount = 1 To Windows(fName).Application.Sheets.Count  ''''Required to search each sheet to find the Cansim number''''
            With Application.Workbooks(fName).Sheets(iCount).Range("A1:Q80")   ''''The range from the source to search in''''
                Set rngName = Application.Workbooks(fName).Sheets(iCount).Cells.Find(cell)    ''''Set the Range if the Cansim number is found''''
                If Not rngName Is Nothing Then  ''''If the Cansim number is found, perform the following actions''''
                    'MsgBox (cell)  ''''This is for testing purposes only''''
                    Set dFind1 = rngName.Offset(1, 0).End(xlToRight).End(xlDown)
                    numCount1 = 0
                    Set dFind2 = dFind1.Offset(0, -12).Resize(, 12)    ''''Select the data range''''
                    For Each dataFind In dFind2    ''''Find the data to be transferred''''
                        If dataFind.Value <> ".." Then
                            numCount1 = numCount1 + 1   ''''Count each cell that has data in it''''
                        ElseIf dataFind.Value = ".." Then
                            Set dSelect1 = dataFind.Offset(0, -numCount1).Resize(, numCount1)  ''''Set new range with only data values''''
                            With Application.Workbooks("IndustryPriceIndex.xls").Worksheets("Mthly")
                                cell.End(xlToRight).Offset(0, -((numCount1) - 2)).Resize(, numCount1).Cells.Value = dSelect1.Cells.Value   ''''set position to add values into''''
                            End With
                        End If
                        If dataFind.Value = ".." Then Exit For
                    Next dataFind
                End If
                If Not rngName Is Nothing Then Exit For
            End With
        Next iCount
    Next cell
    Application.ScreenUpdating = True
End Sub

It was not easy and took me a fair bit of time to do but I'm glad I did it because I learned a lot while fighting throught the code to make it work. It also reduced the total time to update by about 35 seconds (which seems like forever when you are waiting for code to finish).

Hi all,

Andrew, I agree, "never stop learning"

Mordred,
I'm about to take the thread slightly off track again - the joys of the Watercooler eh?

Originally Posted by Mordred

...It was not easy and took me a fair bit of time to do but I'm glad I did it because I learned a lot while fighting throught the code to make it work. It also reduced the total time to update by about 35 seconds (which seems like forever when you are waiting for code to finish).

That's a great speed improvement & the code looks better too
How fast is it now?
I think we could make your code even faster with a few more tweaks.
- To save some typing time you could incorporate GetOpenFileName into your code (see here or here for Jacob's examples). However, if you have Conditional Formatting on your sheets then also have a read of this thread & the tinyURL's referring to Greg Wilson's findings.
- Each time you interact between a spreadsheet & VBA slows a macro down. The below code seems to suggest that you are looping through every cell in a Range that is 12 columns wide.

                    Set dFind2 = dFind1.Offset(0, -12).Resize(, 12)     ''''Select the data range''''
                    For Each dataFind In dFind2    ''''Find the data to be transferred''''
                        If dataFind.Value <> ".." Then

It seems that you currently only work on one file at a time and leave that file open even when the macro finishes. If you are not making any permanent changes in the opened file (ie just grabbing the values), then you could:

• open it as read only
• record a macro that Finds & Replaces ".." with "" (perhaps even do this before setting the first range)
• then use a combination of .end(xldown) &/or .end(xlright) to identify the size of the range
• copy/transfer the range values
• then close the file without saving

- It seems as if nothing actually "belongs to" the below line object in the With statement, is it redundant?

With Application.Workbooks("IndustryPriceIndex.xls").Worksheets("Mthly")

- Also, (I think this is mentioned in the "What not to do in VBA" thread), each time you use "Set xyz = ..." in your code, many people recommend releasing the memory at the end of your code by using "Set xyz = Nothing" once you have finished using the object.


hth
Rob

Originally Posted by romperstomper
If you are not using the Call keyword, or returning a value from a Function, do not enclose arguments in parentheses.

Right, I did read that once already didn't I? I'll just slap myself for you since you can't do it from where you are.

Oh yeah?

Sub Smackdown()
Dim usr as ExcelForum.User
Dim addr as String

Set usr = "Mordred"
Set addr = excelform.db.find(usr.last_ip_addr)

With addr
    On Error GoTo ban
    .Connect
    .Run "Turn_On_Electric_Keyboard"
    .Run "Kick_Cat"
    .Run "Open_Refrigerator_Door"
    .Run "Invite_Mother_In_Law_To_Dinner"
    .Disconnect
    On Error GoTo 0
End With
Exit Sub

ban:
usr.BanFromForum Why:="Can't recite PI to the 84th decimal"
End Sub

.Run "Invite_Mother_In_Law_To_Dinner"

Noooooooooooooo!

Hi broro,
I timed it at just over 6 seconds to complete with an online stopwatch so there is a +- of about 2 or 3 seconds. There is no conditional formatting, it is just a straight movement and storage fo data. I am going comment this out:

With Application.Workbooks("IndustryPriceIndex.xls").Worksheets("Mthly")

to see if it makes a difference or not. As for the "..", the values to be updated are each month but, we don't have say August yet so the update from our source uses ".." instead of a numerical value. The user just changed his criteria a little so I am now working on a way to grab all the values in the range except for ".." and 4 other cells in the range. I've been working on a function that will help but it is still in progress. The code I finish with will probably not resemble the code I submitted on this thread.

I really do appreciate your feedback though on that, no doot aboot it!

Best Regards

hi Mordred,

If you want any feedback on your final code, feel free to start a new thread asking for feedback & pm'g me a link to it (or even providing a link in this thread)

Rob

Here is more for the do or don't thread.
While in VB editor, there is the option to insert a new module or, the actual worksheet can have code written into it. I guess my question is, when is it appropriate to insert code into the worksheet? I mostly use modules but I have inserted code into worksheets when I am designing a worksheet. Is there a do or don't for code in worksheets compared to modules? Some insight is appreciated as always.

As a general rule, event code (for the sheet and its objects) goes in a sheet (or workbook) module. Everything else goes in a normal module. There are different schools of thought, but they're wrong.

Originally Posted by romperstomper

There are different schools of thought, but they're wrong.

Thanks romperstomper, lol, I'll take your word as gospel on the matter then.

@romper, nice carving by the way for your avatar. Did you carve it? It is a pumpkin right?

Yes, and yes. Thank you - my daughter wasn't impressed by any of them!

my daughter wasn't impressed

Appreciation of art will not develop before the age of 3.

and that answers what question squig?

oops, blush! erm it answered on question somewhere!

Ok if the post below answers your question then YEY, otherwise oops!

I think you'll find the answer is 42

Dom

and the question then must be "What is your age?!"

I was very disappointed in the final book of that series. Sighhhh

A lot was made previously about using GoTo, but how do you handle errors without using Goto? Or is error handling the one exception?

Error handling is the exception - no real choice there.

Originally Posted by romperstomper

Error handling is the exception - no real choice there.

With the exception of GoTo 0, I have not used a GoTo in years. They can be completely avoided.

You mean you don't use error handling blocks, you just use On Error Resume Next?

Originally Posted by romperstomper

You mean you don't use error handling blocks, you just use On Error Resume Next?

Correct. In effect, error handling blocks are in line with the code. I will use Exit Sub in the block after setting appropraite error flags, if necessary.

I know this is a style thing. I find it easier to assure myself that I have handed error conditions by leaving the hanlding block near the error.

If VBA had try...catch like other languages, I would prefer it. On Error Resume Next the closest I can get.

Originally Posted by blane245

If VBA had try...catch like other languages, I would prefer it. On Error Resume Next the closest I can get.

I would have said that on error goto label was closer to Try...Catch, but hey, it's your code.

Of course that assumes you can predict every single error...

I agree that error prediction is an essential part of coding, but I don't believe it is possible to predict every error that might occur, so I think you should have some generic error handling, even if it's only in the top level sub.

This is purely my opinion, and I've had some "lively" discussions in the past with various programmers (whom I otherwise respect) who disagree with me, so these days I just agree to differ on this point - as long as the error handling isn't simply putting On Error Resume Next at the top of every sub.

As we all know, a fair proportion of coding practice is subjective (and the people on each side of any given argument seem to be equally vehement in general) so as long as I don't have to support other people's code I try to leave them to it. Unless of course, I think they are recommending bad practices to others...

Originally Posted by romperstomper

I agree that error prediction is an essential part of coding, but I don't believe it is possible to predict every error that might occur, so I think you should have some generic error handling, even if it's only in the top level sub.

So you us

On Error Resume Next

for every sub? Dang, I've only used them with forms.

Originally Posted by Mordred

So you us

On Error Resume Next

for every sub? Dang, I've only used them with forms.

On Error Goto label

is better rather than ignore the error handle it!

its also good to start with :-

On Error goto ExitGracefully
Application.enableEvents = false

<Your Code>

ExitGracefully:
Application.EnableEvents = TRUE

I use this any time I use EnableEvents for speed, saves my hair when the code crashes,and I forget to manually turn events back on so then nothing works!, as Enable Events is permenent while ScreenUpdating only lasts until the end of the Sub/Function!

Originally Posted by Mordred

So you us

On Error Resume Next

for every sub? Dang, I've only used them with forms.

Absolutely not. Other than quick and dirty code, I try to only use On Error Resume Next in short single-purpose routines (such as checking if a sheet exists) and use an error handler, rather than suppressor, in main routines (as Darren said).

Well stated. I am in the IV&V business and have done some study into how to determine if error handling is adequate. The research has not yet settled out to any clear means of making this determination. In the meantime, we are stuck with our own opinions.

I have no idea what IV&V is. I know what IV Guinness is, but I suspect that's quite different...

Originally Posted by romperstomper

I have no idea what IV&V is. I know what IV Guinness is, but I suspect that's quite different...

Don't feel bad - there are many who don't have a clue. On the other hand, I know (and love) Guinness, but never heard of IV Guinness.

Some drink Guinness in cans, some in bottles, others in pints. I take it IntraVenously.

Awesome, where can I get some?

What R. said! I will use multiple Resume Next/GoTo 0 segments if I am trying to trap multiple different errors in the ame routine, but never leave a Resume Next open for more than a single statement and its error handler.

So then I do not have to go into all of my modules and the On Error Resume Next. Good to know. I know it is unlikely to catch all errors but I like to use If statements when I know that an error happens. In my own opinion, taking the time to test code in regards to the process it is to perform is the best way. It is time consuming (and something that most employers do not like) but it is an effective approach to code design.

FYI, this discussion is very educational for me. I've been teaching myself VBA (with the help of everyone here) and most of my brainpower has gone to wrapping my brain around the code. I'm just now starting to look at error handling. Thanks to all.

If it's an education for you I feel like I'm in primary school

Dom