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 your
Function 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
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
lastRow = .Range("C" & .Rows.Count).End(xlUp).Row
lastColumn = ActiveSheet.Cells(ONE, Columns.Count).End(xlToLeft).Column
Worksheet object has a property that is much easier to use and much more reliable and readable in your code:
lastColumn = .Columns.Count
lastRow = .Rows.Count
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
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:
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
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
Right() into a
String, use the string specific functions
Right$() to prevent casting back and forth from a
Variant. Better than
Left$(Right$(var, start), length) would be to use the
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)
sheet.Cells(row, col).Value = vbNullString
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