Thread: What to Avoid in VBA

I stand by my recommendation.
Using "For i = j to k" is ok for homework, but is a bad habit to get into for production work.
ALWAYS assume your little bit of code will be built upon, expanded, or become part of a larger project. A few seconds spent up front could save hours later.

I've had to clean up too many instances of other people's code that was like this:

For i = j to k Step p
    For x = y to z
        If a > b Then
            '....many lines of code
        Else
            '....many lines of code
        End If
    Next
Next

50 lines into the code, my head is spinning when I run into a line like:
"If i < z Then"

I see. You still have a global variable but it's more obvious and with centralised control over possible bad states.

For scoping I keep it as local as possible with prefixes to identify the scope as well as variable type.

strA        ' local to routine
m_strA    ' local to module
g_strA     ' local to project

I agree with you that using some sort of notation like the hungarian notation would help maintain (and understand) the code.

... and do we now ALL promise to use this in our Forum answers ? or do we consider that as homework?

It's homework until I get my honorary doctorate from the University of Excel!!!

Dom

I agree to continue using descriptive variables in posts.

In our responses, we are teaching proper methods for solving Excel issues.

While inefficiencies in formatting, functions, and sheet structure are usuallly easy to correct,
poor programming habits in small projects turn into poor programming in large projects
that can be a nightmare to fix. Best to prevent them at the beginning.

Originally Posted by RichardSchollar

I bet you wouldn't really use a variable named "str", Tony

Hmmm

    str = str & " where c2.IsReal=""Y"" and [c2].[Include] = true " 'don't include non partners
    Set rstAllPartners = db.OpenRecordset(str)
    IsFirstPass = True

But you see it is only useful very locally (in this case for one line). Sometimes it takes several lines to compile the select statement.

I think Richard was referring to the fact that str is a native VBA function...

Originally Posted by tony h

There are a number of advantages (in my mind). By using a class module I tend to call it myGlobals and using properties then:
1. it is quite clear that you are using a global value as the syntax is myglobl.TaxRrate
2. it is easy to trap where the global is used as you can put a breakpoint on the property in the class module
3. when dealing with anything that can go out of scope or lose connectivity (eg connecting to databases especially when updating a spreadsheet) it is easy for the DB objects to be outof scope. By using the myGlobals class the error trapping can be done and the reinstating of the connection effectively automatically by keeping all the code in one place.
4. This also helps with creating logfiles.

and probably a few more

5. and usng the class terminate you can ensure a proper close down of connections

6. which is also a good way of trapping unexpected "out of scope" conditions.

Originally Posted by David

Range.Replace.

If you do find or replace Within Workbook outside of VBA, then run range.replace in VBA, it replaces in whole workbook.

Originally Posted by Dom

I was told when using Find and Replace in VBA it's a good idea to specify all the arguments every time. Not doing so can cause problems similar to what you have eluded to.

The error occurs when you do a Replace across the workbook (from the UI, per force), then a range.Replace from VBA. The range is ignored and replacement is performed throughout the workbook. Specifying all the parameters of the Replace won't help; you have to do a dummy Find to reset the behavior:

Set r = Worksheets(1).Range("A1").Find(What:="Texas", LookIn:=xlFormulas)

The word Texas is key here ...

Don't know if this has been corrected.

Here's my list:

1) Variables

    a) Pick a variable naming convention and follow it. 

    b) Declare all variables appropriate to usage. 

    c) Use Variants only when necessary, to wit:

        i)   To receive an argument that may be of varying type 
             (string, Boolean, number, or arrays of same)

        ii)  To receive a value from a cell than may be of varying type

        iii) To receive the value of a multi-cell range from a worksheet

        iv)  To receive an indefinite number of arguments (ParamArray is a variant)

        v)   To receive the results from many worksheet functions, like MMult (I'd have 
             thought that a dynamic array of Doubles would work, but I couldn't get it to)
             and Array

        vi)  To receive the value of a property that may return Null 
             (e.g., Selection.Interior.ColorIndex for a non-homogeneous selection)

        vii) To receive the keys of a dictionary

        viii) To be the control variable of a For Each loop when not a more closely-aligned 
              data type is available (e.g., a Range)

        ix)  To be the return type of a function that returns data of varying type, 
             or (on Mac, I’m told) an array of variable size 

2) Don’t select cells, sheets, workbooks, or other objects except as necessary

3) Code structures:

    a) Generally, use built-in (worksheet) functions when possible
       (there are some exceptions).

    b) Don’t use loops if there’s a way around it; if there isn’t, prefer iterating elements 
       from a collection (For Each/Next), followed by For/Next and Do/Loop. 

    c) Select Case statements are prettier, If/ElseIf/Else/EndIf statements are faster (curious; you'd think the compiled code would be the same). Either should be arranged to test cases from most likely to least.

    d) Use collections and dictionaries in preference to other data structures when possible.

    e) Don't use Option Compare Text; there are better options. 

    f) Use the Like operator sparingly. Ditto for Regular Expressions. Avoid text comparison entirely when there are other options (e.g., TypeOf  versus TypeName)

    g) The Len function is among Excel's fastest; all it has to do is grab the first word of
       a string, where the length is stored. So rather than If s <> "", use If Len(s) (and 
       there’s no need to coerce a number to a Boolean so it can be converted back to a
       number)

    h) Use early binding for objects unless you have specific reason not to.

    i) Write logical, clear code first, and then optimize for speed. Don’t obscure the 
       structure and intent of code for speed unless you don’t mind having the person 
       that has to maintain it later casting aspersions on your lineage.

    j) Comment your code minimally beyond the procedure level; it’s impossible to keep 
       code and comments in sync. Instead, make code shine in clarity and simplicity.

4) Speed

    a) Set Calculation to manual during execution. If worksheet calculation is necessary 
       in the course of macro execution, calculate only the minimum range necessary.

    b) Turn off ScreenUpdating during execution

    c) Turn off events if necessary to avoid triggering change event macros and getting 
       other listeners excited

This thread has started humming since I last saw it!

Shg, I can't remember having ever seen a code snippet on the Forum that uses TypeName or VarType etc & I'm going to have go investigating now!

I may have missed this in amongst the posts, but what is the general consensus on explicitly naming arguments vs using commas as placeholders in argument listings?
For example:

MsgBox "done", , "FINISHED"
MsgBox prompt:="done", Title:="FINISHED"

I try for the latter, but I know I'm guilty of the former, & it's usually when I can see the OP is likely to have some understanding of the code already. In other words, I'm probably reinforcing existing bad habits & showing the new folk to "do as I say rather than do as I really do"...

Rob

Just be aware that TypeOf and TypeName don't do the same thing and TypeName is better in certain situations.

Just be aware that TypeOf and TypeName don't do the same thing and TypeName is better in certain situations.

Indeed, and I believe you gave me an good example with controls some weeks ago.

Maybe this is a better example of avoiding a text comparison:

    Set wks = Worksheets("Sheet1")
    If wks Is Sheet1 Then MsgBox "True"         ' is better IMO than
    If wks.CodeName = "Sheet1" Then MsgBox "True"

Thanks Shg & RS,
"TypeOf" is my little learning for today - it seems to be hidden away in the 2007 Help files under "If...Then...Else Statement".

My investigations/Google shows an incy squincy total of about 100 results in the Forum for the below three data information functions: http://www.google.co.uk/search?hl=en...meta=&aq=f&oq=
Could/should we be more defensive when offering solutions?

StephenR, I'm pleased I'm not the only inconsistent one - your logic is sound too.

expand on your reasoning behind

prefer iterating elements from a collection (For Each/Next), followed by For/Next and Do/Loop.

and

Use the Like operator sparingly. Ditto for Regular Expressions.

My understanding is that iteration and search of collections and dictionaries are very fast and highly optimized. They are also very intuitive stuctures, IMO. Beats the heck out of redimensioning dynamic arrays in a loop.

RegEx and it's little brother, the Like operator, entail complex comparisons; think about the difficulty of writing one, and then stop when you feel a blood vessel approaching the bursting point in your brain. They're great tools, just limit their use to when they are justified.

Like is so easy to use that I abuse it, I confess.