3
\$\begingroup\$

Working on finding a way to reduce in-cell formula size and memory usage of my excel workbook, i came up with the idea to "load" (copi-paste) the reference sheet i was mentioning a lot with a workbook open. To make it more efficient i had a loop that verifies if the sheet has changed and only copy-paste it if there a difference. I also added a LinkBreak because i didnt want the link from the ref sheet to slow the workbook i was importing it on.

The code has most likely a lot of flaws : i managed to get it working but it still doesn't feel right. Maybe you'll be able to propose me a solution.

As of right now, one of my files is working with that code in the factory, the rest are all working with the "old" outer links.

Sub Workbook_open()
' This sub loop trough the external wb reference sheet and compare it to the last internal ref sheet copi
' If it spots a difference it copies the ref sheet from the external file onto the internal file samely name sheet
    Application.ScreenUpdating = False
    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
    Sheetname = "cédule détaillée 2 "
    
    strRangeToCheck = "A1:AE40000"    ' Estimated check range

    Debug.Print Now     ' Don't know what it does come w the code i found online

    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
    varSheetA = wbka.Worksheets(Sheetname).Range(strRangeToCheck)  ' Set outer file ref sheet
    varSheetB = Worksheets(Sheetname).Range(strRangeToCheck) ' Set inner file ref sheet
    wbka.Close False
      
        Debug.Print Now
    
    Dim Z As Integer: Z = 0     ' My different or same boolean (could prob be an actual boolean)
    
    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
        
    If Z = 1 Then    ' if my boleen = 1 then means its different
    
        Application.EnableEvents = False
        Worksheets(Sheetname).Visible = True  ' Make hidden sheet visible
        
        Dim wb As Workbook
                    
        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
        wb.Activate
        lastCol = Sheets(Sheetname).Range("a1").End(xlToRight).Column
        Lastrow = Sheets(Sheetname).Cells(1, 1).End(xlDown).Row
                    
        Sheets(Sheetname).Activate
        Cells.Select                   ' copi -paste (i will not comment that)
        Selection.Copy
        ThisWorkbook.Activate
        Sheets(Sheetname).Paste
        wb.Close False
                
            With ThisWorkbook                                             ' link breaker code
                For Each lnk In .LinkSources(Type:=xlLinkTypeExcelLinks)
                .BreakLink Name:=lnk, Type:=xlLinkTypeExcelLinks
                Next
            End With
            
        Worksheets("Sheetname ").Visible = False              're hiding sheet
        Application.ScreenUpdating = True
    Else
    End If
    


  ' **Note that il probably divide these 2 parts in 2 subs that are called by workbook_open to be clearer/cleaner**
' This part loop to find the sheet that has the same date as the actual date
        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
                
        
        Application.EnableEvents = True
        Application.ScreenUpdating = True
        
        End Sub
\$\endgroup\$
6
\$\begingroup\$

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.

Danger Will Robinson, Danger!!

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.

  • Dim varSheetA As Variant is slow and confusing to both the programmer and the RTE

  • Dim ExternalRefSheet As Excel.Worksheet is fast and explicit to both the programmer and the RTE

    • Note that the Excel. portion isn't absolutely necessary, but it never hurts to be as explicit as possible - it removes the possibility of confusion.

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:

enter image description here

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.

\$\endgroup\$
9
  • 2
    \$\begingroup\$ Relax @PatatesPilées, we all have those moments - sometimes even when we know better... That's why we have code reviews, to make sure that none of our brain-dead moments make it into production and blow things up! \$\endgroup\$ – FreeMan Jul 31 '20 at 15:42
  • 2
    \$\begingroup\$ I'm glad you asked! Rubberduck is an awesome OSS program that I use extensively. It'll do indentation for you (and to your own rules), as well as help catch all sorts of programming errors. #Disclaimer: Big fan. Regular user, Small contributor. \$\endgroup\$ – FreeMan Jul 31 '20 at 15:44
  • 2
    \$\begingroup\$ DuckDuckGo: MSDN VBA Now. Also, search around this Q&A, I believe there are one or more timer routines that people have posted. \$\endgroup\$ – FreeMan Jul 31 '20 at 17:00
  • 1
    \$\begingroup\$ @PatatesPilées note that there are some new thoughts on comments in the newly renamed More Efficiency section. \$\endgroup\$ – FreeMan Aug 17 '20 at 18:44
  • 1
    \$\begingroup\$ Btw i've reworked your refactor code; i actually only needed rowaredifferent as comparaison (Sorry for all that big loop correction) and im still using copy-paste (there was an issue with deleting the sheet) but without either activate ou select ;) Il keep that knowledge in mind tho \$\endgroup\$ – Patates Pilées Aug 17 '20 at 18:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.