Excel 2010: Seeking help to improve my macro and coding skills

[GomezAddamz]

Hey forum! I've written a macro for Excel 2010 for work. It's perfectly functional. It does everything I need it to and it doesn't have any problems. It is, however, clunky. At least it seems that way to me. I'm pretty new to VBA (for Excel in particular), and am interested in getting better. I was hoping to use this example as an opportunity to improve. I'd appreciate it if you guys could take a look at this and let me know what you think. Any and all tips, ideas, feedback, etc. is greatly appreciated.

This is a macro that will update an Excel file (current data) based on the contents of a semi-colon delimited text file (new data). If a current entry is not in the new data, it can be marked as complete. If a current entry is in the new data, it can be ignored. Here's how the code is structured.

Part 1 loads new data and initialize variables. 'j' is the index of the last 'Completed' entry, which is stored on its own sheet.

Part 2 moves 'Completed' entries out of current data. Column 'E' contains a unique identifier which is used to compare the current data to the new data. This loop takes every entry in current data and looks for it in new data. If the current data is found in the new data, it moves on to the next entry. If the data is not found, it moves it to the Completed sheet. 'i' is the index for the current data, which begins on the second row, and 'r' is the index for the new data, which begins on the fifth row.

Part 3 ignores new data that is already in current data. Basically it does the reverse of the last section. This time, it takes every entry in new data and compares it to the current data. If the new data already exists in the current data, the entry is discarded. Otherwise, it is left alone.

Part 4 adds the new data to the current data, and part 5 closes files and displays updated results.

I had the relevant sections of code beneath their descriptions, but that looked a bit weird in the preview. I've marked the different parts in the code below, but they shouldn't be too hard to associate. As you can see, I've used a lot of Do Until loops. I started by using For loops, but then I needed to know how many iterations before entering the loop, as opposed to the more dynamic nature of the Do loop. Also, I haven't really found a good way to find the last entry on a worksheet.

We're going to have about 50 different spreadsheets that will have this macro. There should not be a situation where there is a lot of data (1000+ entries) in either the current or new data. As I say, as it is, it's working for what it needs to do, but from a coding/standards standpoint, I'd like to know what I could do to make it cleaner and maybe break some bad habits before they form. Let me know if you have any questions. Thanks!

Sub UpdateReport()

'Updates Excel spreadsheet based on imported text file

Application.ScreenUpdating = False

'Part 1: Initialize variables

dups = 0
dels = 0
adds = 0
recs = 0

'Set filepaths for new and current workbooks

currBook = Application.ActiveWorkbook.Name
openFile = Application.GetOpenFilename("All Files (*.*),*.*,Text Files (*.txt), *.txt", 2)

If openFile = False Then
    MsgBox ("Update Failed. File may be in use.")
    Exit Sub
End If

Workbooks.OpenText Filename:=openFile, DataType:=xlDelimited, Semicolon:=True

newBook = Application.ActiveWorkbook.Name

'Find the last completed entry

j = 1

Do Until Workbooks(currBook).Sheets(2).Range("E" & j).Value = ""
    j = j + 1
Loop

'Part 2: Update current data based on new data

i = 2
fin = Workbooks(currBook).Sheets(1).Range("E2").Value

Do Until fin = ""

    found = False
    r = 5
    dataCells = Workbooks(newBook).Sheets(1).Range("E5").Value

    Do Until dataCells = ""
    
        If dataCells = fin Then
            found = True
            Exit Do
        End If

        r = r + 1
        dataCells = Workbooks(newBook).Sheets(1).Range("E" & r).Value

    Loop

    If found Then
        i = i + 1
        fin = Workbooks(currBook).Sheets(1).Range("E" & i).Value
    Else
        Workbooks(currBook).Sheets(2).Range("E" & j).EntireRow.Value = Workbooks(currBook).Sheets(1).Range("E" & i).EntireRow.Value
        Workbooks(currBook).Sheets(1).Range("E" & i).EntireRow.Delete
        Workbooks(currBook).Sheets(2).Range("N" & j).Value = Date
        j = j + 1
        dels = dels + 1
        fin = Workbooks(currBook).Sheets(1).Range("E" & i).Value
    End If

Loop

'Part 3: Remove new data that is already in current data

r = 5
dataCell = Workbooks(newBook).Sheets(1).Range("E5").Value

Do Until dataCell = ""

    recs = recs + 1
    i = 2
    fin = Workbooks(currBook).Sheets(1).Range("E" & i).Value

    Do Until fin = ""

        If dataCell = fin Then
            Workbooks(newBook).Sheets(1).Range("E" & r).EntireRow.Delete
            dups = dups + 1
            r = r - 1
            Exit Do
        End If

        i = i + 1
        fin = Workbooks(currBook).Sheets(1).Range("E" & i).Value

    Loop

    r = r + 1
    dataCell = Workbooks(newBook).Sheets(1).Range("E" & r)

Loop

'Part 4: Add new data to current data

i = 1

Do Until Workbooks(currBook).Sheets(1).Range("E" & i).Value = ""
    i = i + 1
Loop

r = 5

Do Until Workbooks(newBook).Sheets(1).Range("E" & r).Value = ""
        Workbooks(currBook).Sheets(1).Range("E" & i).EntireRow.Value = Workbooks(newBook).Sheets(1).Range("E" & r).EntireRow.Value
        Workbooks(currBook).Sheets(1).Range("M" & i).Value = Date
        i = i + 1
        r = r + 1
        adds = adds + 1
Loop

'Part 5: Clean up and report

Application.ActiveWorkbook.Close (False)

Application.ScreenUpdating = True

junk = MsgBox("" & recs & " records processed. " & vbNewLine & _
            adds & " records added," & vbNewLine & _
            dels & " records removed," & vbNewLine & _
            dups & " records omitted.", vbOKOnly, "Update Results")

End Sub

 

[SkipVought]

hi,

Some thoughts on your code.

I NEVER use the Workbooks.OpenText method for TEXT files. As a result, you have absolutely no control over possible unintended conversions that Excel WILL MAKE. Rather, I use the Data > Get external data > From Text File.... within a sheet. You can specify the data type of each column, ESPECIALLY date columns, where the structure of dates represented in TEXT and vary all over the place.

Regarding the last row/column of a table on a sheet, assuming that your table is the only data on the sheet. Check out either the UsedRange property or the CurrentRegion property. The caveat regarding the UsedRange property, is that if you simply delete rows/columns (as in the [Delete] key) which is similar to the ClearContents method, that 'deleted' range still retains DATA properties, and will be 'seen' by UsedRange. If you really want ALL DATA to be removed, you must CLEAR the range, akin to SELECT > Right-Click - Delete - Shift Cells. Here is an example of some last whatever techniques...

With SheetObject.UsedRange
  lFirstRow = .Row
  iFirstColumn = .Column
  lLastRow = lFirstRow + .Rows.Count - 1
  iLastColumn = iFirstColumn + .Columns.Count - 1
End With

I ALWAYS position my tables starting in A1. I ALWAYS have unique values for headings in row 1. So here's an example using the CurrentRegion property

With SheetObject.Cells(1,1).CurrentRegion
  lFirstRow = .Row
  iFirstColumn = .Column
  lLastRow = lFirstRow + .Rows.Count - 1
  iLastColumn = iFirstColumn + .Columns.Count - 1
End With

Most of my tables have at least one column that contains no empty cells. So here's an example of the Range.End property, akin to using [End] [Down Arrow] in a range.

  lLastRow = SheetObject.Cells(1,1).End(xlDown).Row
  iLastColumn = SheetObject.Cells(1,1).End(xltoRight).Column

I might use this method like this...

  Dim r as Range

  For Each r in Range(Cells(2,1), cells(2, 1).end(xlDown))

  Next

If you have Excel 2007+, you have the Structured Tables feature (Insert > Tables > Table) and any QueryTable you might have on a sheet. If, for instance, Column 1 heading were Name and the table name were Table1, then the loop would look like this to loop thru every name in the table

  Dim r as Range

  For Each r in [Table1[Name]]

  Next

Otherwise, for Excel 97-2003, I'd use Named Ranges, Naming the ranges according to Create Names in the TOP ROW.

    Application.DisplayAlerts = False
    SheetObject.[A1].CurrentRegion.CreateNames True, False, False, False
    Application.DisplayAlerts = True

I personally don't like the idea of MOVING or DELETING data. If I want a subset of data from one sheet (table) 'moved' to another, I'll use MS Query via Data > Get External Data > From other sources > From Macrosoft Query... and drill down to my workbook as the data source.

We're going to have about 50 different spreadsheets that will have this macro.

BIG mistake, IMNSHO! Put your module(s) in your PERSONAL workbook that ALWAYS opens in the background. Make sure that you use specific workbook & worksheet references in order for you code to work in ANY similarly structured workbook. That way you have ONE source sode to maintain, not 50!!!

Oh, yes and use Option Explicit, which you can set in your VB Editor Tools > Options. EVERY variable needs to be explicitly declared!





Skip,
[sub]
Just traded in my old subtlety...
for a NUANCE![/sub]

 

[GomezAddamz]

Thank you very much! That is very helpful!

Regarding the 50 files, it's a bit hard to explain, but simply put, my department was asked to help out with this project. We won't be using these files, we'll be creating a file for each of about 50 different groups for them to use. A lot of the people who will be using them have little to no technical savvy, hence the automation. I know it sounds weird, but that's how they set it up, and that's how they asked us to do it.

Out of curiosity, any thoughts on a Do loop vs a For loop? Not sure how, but somewhere during my travels, I got it in my head that the For loop was preferred. Is one inherently better than the other (or considered 'cleaner' than the other), or is it just that For loops are good for when you have a known number of iterations and Do loops are good for when the number of iterations may fluctuate?

Thank you again for your help. I'm going back through my macro with this info and it's a big improvement.

 

[SkipVought]

My dismay was not necessarily that there are 50 workbook, but that EACH ONE HAS MACROS! That can be a maintenance nightmare!

When I am processing ranges on a sheet, I use For EACH...Next. If I am processing an array, I often use For...Next. There are other times when a Do...Loop is appropriate, for instance if I am using the FIND method within the loop. All depends on the circumstance, just like a carpenter might use one of several saws or hammers in his toolbox.

Skip,
[sub]
Just traded in my old subtlety...
for a NUANCE![/sub]