These items won't necessarily address Excel's memory usage. I think you'd have to have a chat with MS's Excel development team to really address that. This will, however address some of the general coding issues I noticed that should help us write easier to understand and maintain code.
Debug.Print Now 'Don't know what it does come w the code i found online
This is, frankly, more than a little worrying. If you don't know what the code does, either figure it out or don't use it! This one is very innocuous, and isn't anything to worry about, but you could blindly paste buggy, or worse, harmful code in and run it without knowing what you're doing.
I'd recommend using your favorite search engine with the terms msdn vba <unknown function> to find out what that function will do, something like this:
msdn vba debug.print
That'll take you to the MSDN documentation for the Debug.Print command. Do the same for Now to find out what it does.
I'd guess, though, that you found more than just that one line online and blindly copied it in. I'll admit, I'm not always the best at figuring out what code does just by reading it (and the docs for unfamiliar commands), sometimes I have to step through it with the IDE's Locals window open to follow the execution path and see what variables are changing and how. That's OK if you're running in debug in a development environment and you're really taking your time to understand each line (hopefully before it executes and deletes stuff or sends all your private info to someone else), but don't just blindly paste, cross your fingers, and hope for the best. Please...
Variables
Dim varSheetA As Variant 'External file ref sheet
Dim varSheetB As Variant 'Internal file ref sheet
Dim strRangeToCheck As String
Dim iRow As Long 'Row count
Dim iCol As Long 'Col count
Dim Sheetname As String
We're using variables to hold references to the worksheets we're working with and that's awesome! Many just use ActiveWorksheet which has all sorts of inherent dangers. We've avoided that pitfall, but introduced others.
Hungarian Notation
Hungarian Notation is bad (or at least useless) the way most people use it. (Go ahead, take a few minutes to read that to find out why, I'll wait.)
varSheetA I have no need to know that this variable is a Variant when I'm working with it. I really don't.
iRow is misleading! We've claimed that it's an integer i (or an Apple product), yet it's actually a Long (which is better than an Int for any modern version of VBA, so stick with Long).
Naming
I don't know what your variables are for without reading the comments, so make the comments the variable names! Let's rename our variables to indicate what they're actually for - it makes it much easier for a future programmer (maybe even future us) to follow our code.
varSheetA... well... I'm pretty sure that's a Worksheet, but which one? Let's scroll to the top to take a look at the comment to remind us
ExternalRefSheet is a worksheet, and 400 lines of code later, we still remember that it's the External Reference Worksheet without having to scroll around to find it!
iRow is a bit of a conundrum.
- The
i is an unnecessary lie, leaving Row
- but
Row is a property of Worksheet, so it's a bit redundant and confusing. What do to...
- How about
currentRow or possibly externalRow or internalRow? These will make it very explicitly clear which row we're referring to!
- They also have the added benefit of avoiding the possibility of accidentally doing
Dim row As Long which, due to VBA's "helpfulness", will manifest later as ActiveWorkbook.row instead of ActiveWorkbook.Row (notice the difference in capitalization).
Efficiency
Avoid Variant variables whenever possible! Each time our code references a Variant, the VBE run-time engine has to analyze the data in the variable to determine what kind of data it is, then it can determine if it's legal for the function we're trying to use it for. This takes time. Maybe not a huge amount of time, but it adds up, especially in loops.
There are some times when you do need to use Variant, but holding a reference to a Worksheet isn't one of them.
Option Explicit
We were doing so well, declaring variables, then we get to
Set wbka = Workbooks.Open(fileName:="\\Backup\Opérations\Coaticook\Planification\Cédule détaillées\Cédule détaillées des composantes.xlsx") ' Set outer file wb
I scrolled up to see what wbka is (an unintuitive name, possibly a typo?), and discovered that it isn't declared anywhere! VBA so "helpfully" declares it on the spot for us as a Variant. We've already discussed the evils of Variant, so we'll leave that behind. We've also discussed the necessity of naming variables so comments aren't necessary, so we'll ignore that, too.
How do we handle this? Put Option Explicit at the top of every code module you write! When you do this, the IDE will not compile your code unless you Dim every variable you use.
- This will prevent VBA from automatically declaring slow
Variant variables for you
- This will prevent typos from becoming new variables and becoming bugs!
Set wbka = Workbooks.Open("MyFile.xlsx")
If wkbka.Name = "MyFileName" Then...
- Without
Option Explicit, VBA will happily declare a new variable wkbka and now you'll have 2 variables and subtle bugs.
- With
Option Explicit, VBA won't compile this code and it will point to that 2nd line and tell you that it doesn't know what wkbka is, finding the bug for you!
Setting Option Explicit is so critical that MS actually gives you a way of defaulting it on in every code module! In the IDE, select Tools from the menu, then Options, then on the Editor tab, make sure that Require Variable Declaration is checked:
Why that's not the default setting is beyond me, but, that's what it is...
Lies!!
Hey, wait a minute! Those variables lied to me!!!
varSheetA = wbka.Worksheets(Sheetname).Range(strRangeToCheck) ' Set outer file ref sheet
varSheetB = Worksheets(Sheetname).Range(strRangeToCheck) ' Set inner file ref sheet
Those sure look like they should be referencing Workbooks, but, in fact, they're actually referencing Ranges of worksheets! Again, let's fix up the naming and the data type so we know exactly what they're for and don't need those comments.
Dim externalRange as Excel.Range
externalRange = wbka.Worksheets(Sheetname).Range(strRangeToCheck)
Dim internalRange as Excel.Range
internalRange = Worksheets(Sheetname).Range(strRangeToCheck)
Now we know exactly what we've got our hands on without unnecessary comments.
Pot luck
Not really sure what to call this one...
Dim Z As Integer: Z = 0 ' My different or same boolean (could prob be an actual boolean)
That pretty much hits all the points above.
- It's poorly named.
- Why
z? I think of the vertical axis in 3D space when I see z.
- Why
Integer?
- 32-bit VBA (in use since about Office 95) will automatically internally convert all
Integer to Long, so just use Long to begin with and eliminate Integer from your vocabulary and code.
- Note that there are some exceptions - like when you call core Windows DLL functions and it's expecting 16 bits, you'd better pass it an
Integer, but that's a bit of an advanced topic.
- Explicit is good, but
Z = 0 is actually unnecessary
- VBA will helpfully initialize all variables to their 'initialized' state when you
Dim them, so it's a bit redundant
- I'm torn on this one, it's programmer preference
- Multi-line statements (
:)
- They do make for more compact code, but they make the code less readable.
- It takes more cognitive energy for the human to parse the code to figure out what's going on, though the compiler certainly doesn't care.
- The harder it is to read our code the harder it will be for a future programmer (maybe future you) to safely modify it.
- Again, programmer preference, but I'm against it!
- More lies!
'My different or same boolean Well, why don't we say so??- Give it a good name! How 'bout
rowsAreDifferent?
- While we're at it, let's actually declare it as a
Boolean instead of just noting that this is how we're using it?
- Talk about readable code!
If Z = 1 Then ' if my boleen = 1 then means its different
becomes
If rowsAreDifferent Then
- Now there's no need for comments and there is no confusion about what's just about to happen!
More Efficiency
Hey, let's find some efficiency in our code. Looping through cells in a worksheet is one of the more inefficient things we can do. It's much more efficient to copy them to an array and loop through the array. Excel is just slow when it comes to accessing the contents of myWorkbook.cells(myRow, myCol), but it's really quick to access myArray(myRow, myCol) so let's do that.
For iRow = LBound(varSheetA, 1) To UBound(varSheetA, 1)
For iCol = LBound(varSheetA, 2) To UBound(varSheetA, 2)
If varSheetA(iRow, iCol) = varSheetB(iRow, iCol) Then
' Cells are identical.
' Do nothing.
Else
Z = 1 ' My booleen = 1
Exit for ' Exit
End If
Next iCol
Next iRow
Can become this:
Dim externalRange() As Variant 'this is one of those times we actually NEED a Variant!
With externalSheet
'this assumes all rows are filled to the same depth, so the last row in 'A' is
'the last row in every column
'change the way we're finding the last row if this isn't the case
Dim lastRow as Long
lastRow = .Range("A" & .Rows.Count).End(xlUp).Row
externalRange = .Range("A1:AE" & lastRow)
End With
Dim internalRange() as Variant
With internalSheet
lastRow = .Range("A" & .Rows.Count).End(xlUp).Row
internalRange = .Range("A1:AE" & lastRow)
End With
Dim currentRow as Long
Dim currentCol as Long
For currentRow = LBound(externalRange, 1) to UBound(externalRange, 1)
For currentCol = LBound(externalRange, 2) to UBound(externalRange, 2)
If externalRange(currentRow, currentCol) <> internalRange(currentRow, currentCol) Then
rowsAreDifferent = TRUE
Exit For
End If
Next
Next
- We're saving time by looping through an array instead of cells.
- We've found a better, more accurate way of determining the last row in a column.
- We're actually testing for the thing we're looking for (we're looking for the array elements to be not equal so let's test for that) instead of having an empty
TRUE case in our If..Then which requires a comment to tell the world that this is not an error in our code.
- We're setting our newly named
Boolean variable so it's really obvious what we're looking for.
- Oh, one other thing. Notice how the only comments in this code tell us why we're doing what we're doing, not what we're doing?
- Since we vilified
Variant earlier, we add a comment here stating that we know we have to use one, even though they're generally bad
- We've also used a comment to document the logic and assumptions around the
lastRow assignment, but not the how we're getting lastRow.
- Why comments are good - they explain the choices made in doing what we're doing.
- How comments are, generally, bad - they explain the logic in poorly written code that should have been clear enough in the first place.
- There are, of course, exceptions to every rule, but if you have to document how your code works, you should probably take a step back and look at it again. Maybe set it down until tomorrow...
Reopening workbooks
We've determined that we have some different data, so now we reopen the exact same workbook that we've already opened! Way up at the top, we did:
Set wbka = Workbooks.Open(fileName:="\\Backup\Opérations\Coaticook\Planification\Cédule détaillées\Cédule détaillées des composantes.xlsx") ' Set outer file wb
Now we're going to
Set wb = Workbooks.Open("\\Backup\Opérations\Coaticook\Planification\Cédule détaillées\Cédule détaillées des composantes.xlsx", False) ' Open outer wb
To my eye these are the same workbooks. We've already got it open and a variable wbka holding the reference to it, so let's not bother with opening it again. That will save us notable time by not getting it off the network again. That also means that we don't need the Dim wb As Workbook line, and we'll need to change all references below from wb to wbka. To be honest, though, we need a better name than wbka, so maybe we should have renamed that to externalBook or something meaningful. Maybe sourceWorkbook would be better?
Kill .Activate! Kill it with fire!!!
We were doing so well, then we hit
wb.Activate
lastCol = Sheets(Sheetname).Range("a1").End(xlToRight).Column
Lastrow = Sheets(Sheetname).Cells(1, 1).End(xlDown).Row
.Select and .Activate are fraught with potentials for errors and so easy to avoid!.
To get rid of the unqualified Sheets, we make a simple change and we have:
Dim lastCol As Long
'hey, remember we'd already loaded all the data into an array?
'the UBound of that array tells us the (almost) last column count
'the array is 0-based by default, so we have to add 1
lastCol = UBound(varSheetA, 2) + 1
Dim lastRow as Long
'we'll use the same trick as above
lastRow = uBound(varSheetA, 1) +1
Hey wait a minute...
We're copying sheetName from the backup copy into this workbook if somebody changed this workbook. We're .Activateing and .Selecting and .Copying and .Pasteing the whole worksheet. Doesn't Excel have a nifty function to do that? Why... yes, yes it does!
Dim curentSheetNumber as Long
currentSheetNumber = ThisWorkbook.Worksheets(sheetName).Index
ThisWorkbook.Delete(sheetName)
wbka.Worksheets(sheetName).Copy After:=Thisworkbook.Worksheets(currentSheetNumber - 1)
There we go.
- We determined where in the Workbook
sheetName is (in case someone moved it).
- We deleted the whole WorkSheet - it's invalid after someone modified it, right?
- Then we simply copied the whole WorkSheet from the backup version, and even put it back in the same place!
All with as many lines of code as it took to describe the process and no comments necessary because the code is self explanitory.
Breaking links to the past
Hey, this is excellent! Excel is so "helpful" when it provides links back to the original workbook, except when it isn't. Frankly, I might borrow this little loop of code, I like it a lot!
I don't believe, however, that Excel will create the links when we copy the entire worksheet instead of copy/pasting all the cells. Try commenting this loop out to see if it's still needed.
Consistant formatting
The last thing we do is look for today's worksheet. This code is reasonably clean, but it's hard to read and has some small issues. The biggest issue is that the indentation is all over the place. That's been true of some of our other code, too, but it seems most egrigous here.
By reformatting the code a bit, it becomes easier to read:
Dim xSh As Worksheet
For Each xSh In Worksheets ' For every sheet in wb
If xSh.Visible Then ' If visible
Dim sheetdate As Date
sheetdate = xSh.Cells(4.2).Value ' Set sheetdate as the date on sheet
If sheetdate = Date Then ' If date on sheet = the actual date
xSh.Select ' Select that sheet and exit for
Exit For
Else ' else loop till you find it
End If ' else just stay on the sheet it closed on
End If
Next
Right away, we see what looks like a bug:
sheetdate = xSh.Cells(4.2).Value
Even with international settings, I'm pretty sure that the parameter separator is a comma ,, not a period .. So while this will compile, I don't think that's going to work the way we expect it to.
While it never hurts to declare and use a variable, since it does make what we're doing more obvious, if we're only going to use the variable 1 time, it's really optional. That means we could, if we wanted to simplify
Dim sheetdate As Date
sheetdate = xSh.Cells(4.2).Value ' Set sheetdate as the date on sheet
If sheetdate = Date Then ' If date on sheet = the actual
down to
If xSh.Cells(4,2).Value = Date Then
Additionally there's no need for us to have an Else if we're not actually doing anything in that Else clause, so we can get rid of that.
Final thoughts
Since we're now copying the whole worksheet with those 3 simple lines, it's entirely possible that the sheer act of opening the "backup" source sheet from the network will be the longest portion of code execution.
I'd seriously recommend doing some time-based testing to see if just blindly copying the sheet will now be faster than doing all the analysis to determine if anything has changed. I have the funny feeling that it will be faster.
