Comparing vehicle crash data the previous year

I have created an automated macro which takes vehicle crash data from a .csv file and automatically creates a pivot table and charts and compares it to the previous year. The code is approximately 1400 lines long and the data of the uploaded .csv can be anywhere between 2 to 100 MB .csv files with more than 100,000 rows and 36 columns.

The macro runs fine but it makes the machine very very slow and even causes it to crash most of the times. If I tab, to respond to an email, it has a high probability of crashing. Either the macro is continuing to try to do something after running successfully or it is keeping memory tied up after it has finished.

I need a way to optimize this. I have attached the entire macro.

```
Dim YEAR_COL, TYPE_COL As String
Dim CITY_COL, COUNTY_COL As String
Dim DOCNUM_COL, MONTH_COL As String
Dim COUNTY_CITY_COL, CRASH_DATE_COL As String
Dim INJ_TYPE_SERIOUS, INJ_TYPE_FATAL As Integer
Dim G_HEIGHT, G_WIDTH As Integer
Dim G_TOP, G_LEFT As Integer
Dim myColor1(12), myColor2(14) As Long

Dim CURR_YEAR As Integer, PREV_YEAR As Integer
Dim YEAR_NOT_FOUND_MSG As String
Dim INJ_TYPE_NOT_FOUND_MSG As String
Dim CATEGORY_TEXT As String

Dim UPLOADED_DATA_SHEET_NAME As String
Dim CURR_YEAR_SHEET_NAME As String
Dim PREV_YEAR_SHEET_NAME As String
Dim FILTERED_DATA_SHEET_NAME As String, DATA_SHEET_NAME As String
Dim SER_FAT_PLOT_SHEET As String
Dim SER_INJ_DATA_SHEET As String, FAT_INJ_DATA_SHEET As String
Dim SER_INJ_PIVOT_SHEET As String, FAT_INJ_PIVOT_SHEET As String
Dim CHART_SHEET As String

Dim CATEGORY_COL_NAME As String, CATEGORY_COL_NAME2 As String
Dim TOTAL_CATEGORIES As Integer, CATEGORY_TYPE As Integer
Dim SER_UNRESTRAINED_COL_NAME As String, FAT_UNRESTRAINED_COL_NAME As String
Dim ALCOHOL_COL_NAME As String, SPEED_COL_NAME As String
Dim TEEN_DRIVER_COL_NAME As String, OLD_DRIVER_COL_NAME As String
Dim DISTRACTION_COL_NAME As String, MOTORCYCLE_COL_NAME As String
Dim CMV_COL_NAME As String, BICYCLE_COL_NAME As String
Dim PEDESTRIAN_COL_NAME

Doing something like this -

Dim YEAR_COL, TYPE_COL As String

Will dim YEAR_COL as variant and TYPE_COL as string. When you don't define your variable, VBA will declare it as a Variant, which are objects:

Performance. A variable you declare with the Object type is flexible enough to contain a reference to any object. However, when you invoke a method or property on such a variable, you always incur late binding (at run time). To force early binding (at compile time) and better performance, declare the variable with a specific class name, or cast it to the specific data type.

By not declaring variables, you could possibly be paying a penalty.

You need to type both-

`dim YEAR_COL as String, TYPE_COL as String

Or better, don't declare on the same line.

All of these UPPER_CASE_SNAKE variables should be CONST as that's what the upper case snake is for. If they aren't constants, don't violate Standard VBA naming conventions.

You don't need to Call subs, it's obsolete. Instead just use Sub argument, argument

What are all of those MYCOLOR1(i) and MYCOLOR2(i)? It's bad practice to use a number in your variable name and if these are constants - let them be constants. I also don't see the MYCOLOR() array defined anywhere. Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

Why are these set with ""

CATEGORY_COL_NAME = ""
CATEGORY_COL_NAME2 = ""

If they are nothing strings, use vbNullString over "".

Hungarian notation - drop it

Dim strFile As String -> dim fileName as String

Variable names - try to give your variables meaningful names and you won't need the Hungarian notation.

These could probably be switched to SELECT CASE

If CATEGORY_TYPE = 3

It's good practice to indent all of your code that way Labels will stick out as obvious. And it seems like you do have labels, somewhere. Those should be aligned all the way to the left.

Here you did define both types -

Dim col1 As Integer, col2 As Integer

But col1 and col2 tell me nothing about what's happening. Is it sourceColumn and targetColumn or something like thisthingColumn and thatthingColumn?

A huge source of your slow down will be working with data on the sheet rather than behind the scenes -

Selection.EntireColumn.AutoFit

Be sure to avoid things like .Select - it just slows the code down by needing to fiddle with the spreadsheet while doing everything else behind the scenes. There's a good question on StackOverflow addressing this - https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba-macros .

I feel like reiterating this
Give your variables meaningful names

You don't pay per character, so why save 4 with col when it could be column? It's much more clear. src over source? curr over current? You catch the drift.

When working with sheets

Set src = Sheets(sheetName)
Set dest = Sheets(SER_FAT_PLOT_SHEET)

Worksheets have a CodeName property - View Properties window (F4) and the (Name) field can be used as the worksheet name. This way you can avoid Sheets("mySheet") and instead just use mySheet.

Your comments like

' copy formatting

Shouldn't be needed if your code is easy to follow, which it can be. Comments should only be used when you're doing something that needs explaining why you're doing it that way, not what you're doing or why you're doing it.

Comments - "code tell you how, comments tell you why". The code should speak for itself, if it needs a comment, it might need to be made more clear. If not, the comment should describe why you're doing something rather than how you're doing it. Here are a few reasons to avoid comments all together.

Private Sub CreateGraph1(ByRef var As Variant, ByVal chartName As String, ByVal gTop As Integer, ByVal title As String)

and

Private Sub CreateGraph2(ByRef var As Variant, ByVal chartName As String, ByVal gTop As Integer, ByVal title As String)

They have bad names - at least CreateFirstGraph or even better maybe extract a method or function and you won't need to duplicate the code, just call it when needed.

That's all I can really do now and please don't take it harshly. It looks like it's a lot of code from different places cobbled together - which is hard to maintain. But by following naming rules, spacing rules and other best practices, it will be much easier to follow and maintain.

I would also recommend checking out http://rubberduckvba.com/ - it has a lot of useful tools that might help you identify some of these things more easily. It's maintained by some users here at CodeReview - they even have a chat room.

StackExchange Code Review Q#126597, answer score: 6