Best practice for VBA coding - time to clean up a giant project

Hi all...coming for some advice from the community.

I have just finished updating a huge Excel VBA program (38,000+ code lines). I uncovered a mixture of coding practices as the project is 10 years old. I was hoping to get some advice from people.

There are 30+ individual workbooks with references to one master workbook. This master workbook contains dozens of public variable declarations and constants for use by all workbooks.

Data and variable storage
Data is stored and called using a variety of methods. I would like to migrate everything to one method so that redundant approaches can be removed and procedures / functions can be reduced to a common set :

- INI files used for data storage (paths to be used for file open save)
- Registry used for data storage (paths to be used for file open save)
- CustomDocumentProperty used for data storage (relating to the file)
- Public Constant / Variables declarations for sharing values between workbooks
- Class modules used for storage related to the program
- Types used for storage related to program

The use of INI should be replaced with Registry. The use of Class modules vs type vs public declaration...what is the best method to use?

Referencing ranges and objects

Referencing is also a very mixed bag :

- With, End With used for referencing objects
- Range("Worksheet!Hello") string used for referencing
- Workbooks().Worksheets().Range() used for direct referencing
- Set x as Range used for referencing objects
- [A1] short referencing methods used
- .ActiveCell usage throughout

I would say that Setting of objects is the best method for multiple operations. And the use of full Workbook().Worksheet().Range() method instead of constructing a string within Range(). The use of active object should be removed.

Any advice on all of the above appreciated... :)

Surfing around nite< last, I read most of this site: http://www.refactoring.com/

It is all about cleaning up big old code.

It's hard to tell without any sample workbook.
I never like anything pretending 'best', let alone a 'best practice'
I'd prefer helpful suggestions.

- avoid any 'select' or 'activate'; so activeworkbook, activecell, activesheet can be avoided.
- instead of many public variables you can use 1 public array; you can put anything in each element of an array (integer, string, sheet, activeX control, etc.)
- I wouldn't use ini files nor registry, nor documentproperties to store basic information. If you want to reduce inadvertent removal of that information you probably best use documentvariables in a Word Document. (they are invisible and can't be changed 'manually')
- I would refrain form declaring objectvariables, because objects can be handled more easily using the 'with... end with' method (which hasn't been introduced for nothing).

@theta

One master and 30 other WBs -- is the Master the only one with code? Probably not, no one is ever that lucky.

<<Personal Opinion Mode = On>>

For something like that, I'd think first about the architecture of the whole thing first, before I tried to decide if more With/End Withs were needed, etc.

I prefer to seperate the different types of entities some how

1. Control (user commands, system configuration/status,etc.)

2. Data (possibly entry and validation as well as storage)

3. Calculation (possibly by updating other cells or worksheets in the Data)

4. Presentation (reports, pivot tables, charts, a worksheet with JOIN-ed data to make reporting filtering easier, etc.


I typically find that I usually end up going Top Down and Bottoms Up. By that I mean as I worki from the top level, I find that I need a subroutine to do something. I try to make it very general purpose since I know that later I'll probably need to make it smarter to handle something else that I haven't thought about yet. That way I have one sub that will handle many different situations well, instead of many subs that only do one thing.


VERY simple example, in one place I might need to validate that 0<X<100, in another that -10<Y<25, etc. A function: IsXValid(X) works at first, but then later I would need IsYValid(Y).

I try to be alert to oppertunies to make a previous function a little smarter, instead of just quickly writting another one. Instead of writting IsYValid, I'd make the first one IsNumberValid (X, Optional LowVal as long = 0, HiVal as Long = 99999)


Silly example maybe, but the concept works for me, even if it's not 100% effecient and involves re-work.


I ALWAYS go for maintainablity and clarity, even if the code in not tuned to maximum efficency. After all, I'm the person who will have to read the source code in 6 or 24 months and try to figure out just what the heck I was doing

<<Personal Opinion Mode = Off>>


Getting off my soap box now

Paul

Use of Active(whatever object goes here) isn't a bad thing by itself, as you may very well NEED the activecell/workbook/sheet. So whether or not you "should" use ActiveWorkbook depends on your project entirely.

Which method you use to store application settings is up to you, I'd just choose the one you're most comfortable with which is also easiest to implement in the project itself.

Using a (global) class instance to hold app settings is probably easiest as you can encapsulate the storage method in the class so it is easy to change should that be (ever) needed. It is even easy to have multiple storage methods inside the same class, the calling code does not need to know that.

Most developers prefer to use properly declared variables to hold their objects, with ease of maintenance and proper intellisense as a benefit. It also makes re-use of an object simpler.
For example, code like this:
With Worksheets("Foo")
...
End With
deprives you of intellisense on the object.

Placed within a With-End With construct can make your code slightly faster once you need to address more than one property or method of the object in question.

I hate to tell you this now, but "best practices" are best used as a continuous process in the writing of code.

Having to go back through a "giant project" and change all hard-coded sheet references to Worksheet variables is a giant task. Other good practices (e.g. function vs. public variables) have are non-local effects. Change something in one part of the giant project and something off in some small corner is affected. And the developer can't be sure that they found all the small corners.

If you have an existing, working, giant project, it might be best to leave well enough alone and resolve to follow your version of "best practices" throughout the writing of the next giant project.