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)
.