Searching a Word Document from Excel - v2.0

Question

My first question:

Searching a Word Document from Excel

Since then I have implemented most of the suggestions by @RubberDuck and @Comintern. I have also added functionality for different-sized spreadsheets (horizontal and vertical), read multiple files in during a single run of the macro, and a bunch of constants.

This is what I currently have:

Global lineCount As Integer

Sub LoadWordFromExcel()

    Const ONE As Integer = 1
    Const FIRST_ROW As Integer = 2
    Const PATH_TO_FOLDER As String = "C:\Users\<user>\Desktop\Macro folder\"
    Const LAST_NAME As Integer = 3
    Const CHOP_LEFT As Integer = 21
    Const CHOP_RIGHT As Integer = 11

    Dim r As Integer
    Dim k As Integer
    Dim c As Integer        
    Dim filePath As String
    Dim file As String
    Dim lastRow As Integer
    Dim lastColumn As Integer
    Dim sheet As Worksheet
    Set sheet = ActiveSheet

    'Finds last row and column with valid data
    With ActiveSheet
        lastRow = .Range("C" & .Rows.Count).End(xlUp).Row
        lastColumn = ActiveSheet.Cells(ONE, Columns.Count).End(xlToLeft).Column
    End With

    For c = ONE To lastColumn
        file = Dir$(PATH_TO_FOLDER & sheet.Cells(ONE, c).Value & ".docx")
        If (Len(file) > 0) Then
            filePath = PATH_TO_FOLDER & file
            Dim entries As Scripting.Dictionary
            Set entries = LoadWordToDictionary(filePath)
            For r = FIRST_ROW To lastRow
                For k = 1 To lineCount
                    If InStr(entries(k), sheet.Cells(r, LAST_NAME).Value) <> 0 Then
                        sheet.Cells(r, c).Value = Left(Right(entries(k), CHOP_LEFT), CHOP_RIGHT)
                        Exit For
                    Else
                        sheet.Cells(r, c).Value = ""
                    End If
                Next
            Next
        End If
    Next
    MsgBox ("I'm Finished")
End Sub

'Loads data from word document and adds it to a dictionary
Private Function LoadWordToDictionary(file As String) As Scripting.Dictionary

    Dim host As New Word.Application
    Dim doc As Word.Document
    Dim lines() As String

    'loads data from Word doc into lines'
    Set doc = host.Documents.Open(file)
    lines = Split(doc.Content.Text, vbCr)
    doc.Close
    host.Quit

    Dim output As New Scripting.Dictionary
    Dim items() As String
    Dim i As Integer

    'Loads data from lines(array) to output(scripting.dictionary)
    For i = 0 To UBound(lines)
        Call output.Add(i + 1, lines(i))
    Next i

    lineCount = UBound(lines)
    Set LoadWordToDictionary = output

End Function

Questions:

1. Am I still unwittingly using Hungarian Notation? I read the article but it brought up more questions than answers.

2. Is there anything that I'm overthinking and could simplify? I'm looking at my nested for loop and can't help but think there's another way to do it.

So far, this script is FAST. It can enter over 5,000 cells of information in under 30s instead of the 5-10 minutes for 300 cells that the first version did.

Answer 1

Answers

Question 1: No, this is now Hungarian-free™. The article that was referred to in your prior post differentiated between "Apps Hungarian" and "Systems Hungarian", and your previous code was using the latter (i.e. strName). You were basically indicating that yourFunction SearchWordDoc(strPath, strName)was expecting String's as parameters. The current code is both more explicit and will in fact not even compile if you try to pass it anything but a String. This is a Good Thing™.

Question 2: This one is harder to answer without some information about the layout of the spreadsheet. From the code, I would gather that row 1 consists of column headings that correspond to your Word document names and that column "C" is the last names that you are looking up. If this is the case, your first loop (the "c" loop) counter is off - instead of starting at 1, it should start at the first column that is expected to have a value. I'm not sure exactly what the inner-most loop (the "k" loop) is doing - it seems to rely on the Word document being in the same sort order as the rows in the spreadsheet. The Dictionary key is an integer, and you're using the row as the key when you index it instead of the last name. Take a another look at my answer to your last post, you should be parsing a name out of the Word file for the Dictionary keys.

Other Observations

Finding Worksheet Size: The following code will likely return inconsistent results in many cases (at least it does for me):

'Finds last row and column with valid data
With ActiveSheet
    lastRow = .Range("C" & .Rows.Count).End(xlUp).Row
    lastColumn = ActiveSheet.Cells(ONE, Columns.Count).End(xlToLeft).Column
End With

Excel's Worksheet object has a property that is much easier to use and much more reliable and readable in your code:

With sheet.UsedRange
    lastColumn = .Columns.Count
    lastRow = .Rows.Count
End With

Using ActiveSheet: Don't do it. After you get your reference to sheet at the start of Sub LoadWordFromExcel(), don't use ActiveSheet again. If you need a reference in another function, pass it as an argument. Within the same function, always use your captured reference. To expand on my answer to your previous question, every time you call ActiveSheet, you get a reference to whatever Worksheet Excel has active. If you are half way through your routine and the user opens another workbook, your reference can change. This can (and will) result in read garbage cells and writing into the wrong spreadsheet.

Global Variables (other than constants): Avoid them in any way that you can, as they can lead to unexpected and hard to locate bugs. Variables should always have the minimum required scope. In your case, you are using lineCount to track how many items you add to the Dictionary object you return, but there isn't a reason to do this. You can just get a count directly from the object:

'instead of 1 To lineCount ...
For k = 1 To entries.Count 
    ...
Next

Constants: Definately well intentioned, but there are a couple of things to mention here. First, these should typically be placed in your module's header (after the missing Option Explicit). The primary reason to do so is so that they get a wider scope. Remember, the goal of having a constant is not just to have an easily identifiable name - it make changes a lot easier. For example, if your layout changes, you can change Const LAST_NAME As Integer = 3 to whatever value you need exactly once instead of searching all over for the number 3 and trying to figure out if it is being used as a column index. That said, Const ONE As Integer = 1 is never going to change. Use 1 instead. The other goal, easily identifiable names, is the other place I would make a couple minor changes to make them more explicit and readable. I'd likely do something like this:

Option Explicit

Private Const FIRST_DATA_ROW As Integer = 2
Private Const WORD_DOCUMENT_PATH As String = "C:\Users\<user>\Desktop\Macro folder\"
Private Const LAST_NAME_COL As Integer = 3
Private Const PARSE_START_INDEX As Integer = 21
Private Const PARSE_LENGTH As Integer = 11

Variable Names: While you can occasionally use one letter loop counters like i when it is obvious that you are indexing something, in this case it would be better to be more descriptive in what you are looping through and how you are using the loop counters to index. This is much more important in nested loops than in a simple array traversal. I'd suggest something more like this:

Dim row As Integer
Dim key As Integer
Dim col As Integer

For col = 1 To lastColumn
    ...
    For row = FIRST_DATA_ROW To lastRow
        For key = 1 To entries.Count
            ...
        Next key
    Next row
Next col

I'm also a fan of the Next [Variable] syntax for readability, especially with nested loops.

Odds and Ends: The following If test will never return False:

file = Dir$(PATH_TO_FOLDER & sheet.Cells(ONE, c).Value & ".docx")
If (Len(file) > 0) Then

The length of the file variable will always be at least 5 + Len(PATH_TO_FOLDER) - it can never be zero. You should also make a habit of using Trim$() before you test a string length (unless you care about white-space).

Also, if you are going to return a the value of Left() or Right() into a String, use the string specific functions Left$() and Right$() to prevent casting back and forth from a Variant. Better than Left$(Right$(var, start), length) would be to use the Mid$() function.

This test is wildly inefficient:

If InStr(entries(k), sheet.Cells(r, LAST_NAME).Value) <> 0 Then

It takes the returned Item from the Dictionary and then uses a string comparision to see if it matches the cell contents. The Dictionary object has a method to determine if a given key exists - .Exists(). Restructuring this one If statement can likely eliminate the inner-most loop entirely (assuming you key by name).

Dim key As String
...
key = Trim$(sheet.Cells(row, LAST_NAME).Value)
If entries.Exists(key) Then
    sheet.Cells(row, col).Value = Mid$(entries(key), PARSE_START_INDEX, PARSE_LENGTH)
Else
    sheet.Cells(row, col).Value = vbNullString
End If

Finally (and this is more of a pet-peeve of mine), use the built-in constants when they exist to improve readability. "" is much harder to comprehend at a glance than vbNullString in much the same way that vbCr is easier to read than Chr$(13).