4
\$\begingroup\$

Brief:

This is a question geared towards optimization and general better coding practices. I'm self-taught in VBA so I'm cautious of bad practices, but they do come up.

My Reasoning:

I break the links (break_links) because these new files would be sent out and I don't want the external references to cause errors within the spreadsheet formulas. It also makes for better historical files IMO so as the main file is updated we have the historical numbers saved rather than referencing the new numbers.

I did take this component from somewhere online, so I'm still unsure how things work when you put something like (ByRef wb As Workbook) into the sub name ie: when to do it, how to do it, etc.

The Name reference is a cell that has a fiscal year and period labeled for historical files.

My Complaint:

Despite turning off screen updating and all that it still works like a clunky recorded macro where I sit for about 20-30 seconds while it does it's thing, and I'm just exploring to see if there's anything I can do to improve this.

Sub NewWorkbooks()
'This will make seperate workbooks for each of the tabs listed
Dim wb As Workbook
Dim NewBook As Workbook
Dim ws As Worksheet
Dim Name As String
Name = ActiveWorkbook.Worksheets("Reference Tables").Range("_Name")

Call TurnOffFunctions
Set wb = ActiveWorkbook

For Each ws In Workbooks("P&L Master Sheet"). _
Worksheets(Array("Accessories", "Leather", "Residential", "Rugs"))
    ws.Copy
    Set NewBook = ActiveWorkbook
    With NewBook
        Call break_links(NewBook)
        .SaveAs Filename:="C:\Users\Monthly P&L's\Historical Files\" & "P&L " & Name & " - " & ws.Name
        .Close 'SaveChanges:=True
    End With
Next

Call TurnOnFunctions

End Sub
Sub break_links(ByRef wb As Workbook)
Dim Links As Variant
On Error Resume Next
Links = wb.LinkSources(Type:=xlLinkTypeExcelLinks)
On Error GoTo 0

If Not IsEmpty(Links) Then
    For i = 1 To UBound(Links)
        wb.BreakLink _
        Name:=Links(i), _
        Type:=xlLinkTypeExcelLinks
    Next i
End If

End Sub
Private Sub TurnOffFunctions()

    Application.ScreenUpdating = False
    Application.DisplayStatusBar = False
    Application.DisplayAlerts = False
    Application.Calculation = xlCalculationManual
    Application.EnableEvents = False

End Sub
Private Sub TurnOnFunctions()

    Application.ScreenUpdating = True
    Application.DisplayStatusBar = True
    Application.DisplayAlerts = True
    Application.Calculation = xlCalculationAutomatic
    Application.EnableEvents = True

End Sub
\$\endgroup\$
  • \$\begingroup\$ Thanks I'm new enough that I didn't even realize that was it's own thing. How would I go about asking a moderator migrate it? Worst case I can copy and paste this there, but I don't want to have comments lost too. \$\endgroup\$ – Mark S. Jul 31 at 16:19
  • \$\begingroup\$ I flagged it for moderator intervention. If they deem that I'm correct, then they can migrate it over. Thanks for being open to learning how the site works! Again, I could even be wrong and that's the great thing about the way this site is set up. =) \$\endgroup\$ – Robert Todar Jul 31 at 16:22
  • 1
    \$\begingroup\$ Hi @RobertTodar looks like you were right and we got this where it belongs. I have a bunch of different subroutines I wrote where I would love to get some professional inputs, and I just want to make sure everything is where it belongs. Thanks for being helpful and understanding while I learn the ropes! \$\endgroup\$ – Mark S. Jul 31 at 17:29
  • \$\begingroup\$ If the size of each sheet is significant (think 1MB+), you can try using .xlsb format. That does speed up loading and saving, in certain conditions significantly. That's all I have though, so best of luck with your learning and development. \$\endgroup\$ – Filcuk Jul 31 at 17:46
  • \$\begingroup\$ It was my understanding that an .xlsb file format doesn't support macros which is what I'm with here. The full file itself saved as an .xlsm is only 395KB so it's not really a concern. \$\endgroup\$ – Mark S. Jul 31 at 17:49
4
\$\begingroup\$

I do have a few suggestions:

  • First of all, you don't need to reactivate your main workbook every loop, since you hold it's reference in a variable anyway.
  • Second, instead of creating new workbook and copying the sheet over, just make a copy to new workbook (i.e. wks.Copy creates copy in new workbook, you can then assign it to a variable since, as you said yourself, it is now the active workbook)
  • Lastly, unless you want to remove the references (for example if the links contain sensitive information), then I wouldn't worry about it. Excel stores the last value from links in cache within the file.

Depending on your setup, suggestion 2 might actually speed it up a little by reducing the number of actions.

To copy sheet to new workbook, simply ws.Copy, then you can set newWs = Activesheet (or using your naming set NewBook = Activeworkbook) to save it's reference, or just skip that also and put Activeworkbook into your With.

\$\endgroup\$
  • \$\begingroup\$ Hi Filcuk, thanks for your answer. Can you elaborate on how'd I'd go about doing your second suggestion? I would remove the Set NewBook = Workbooks.Add and instead just have the ws.copy and that's it? The references I want to break for security and so if I open the files down the road (or anyone else with access) they don't accidentally update over the historical information (These are P&L statements rather than raw data). Lastly, I'll test this, but I can safely delete the activate line in this instance? Cool! \$\endgroup\$ – Mark S. Jul 31 at 15:51
  • \$\begingroup\$ @MarkS. the only instances I use .Activate is for freezing cells and on the end of a script, otherwise it should be avoided. As to the sheets, see my edit. \$\endgroup\$ – Filcuk Jul 31 at 15:54
  • \$\begingroup\$ I made the change to that section if you care to review. I appreciate your help and effort with this. I'm still new so I'm not sure if this is the type of question you'd mark as answered since I feel like this is one of those 'always improving' type things. \$\endgroup\$ – Mark S. Jul 31 at 16:11
  • \$\begingroup\$ Oh yes, looking at it there's more. You save the new workbook, remove links and then save it again. Instead remove the links first then save and close. I'm also thinking that workbooks may recalculate on save (this happens even with calculations on manual). I'm on the road now, but if you have large ammount of formulas, it may be worth looking into. Again might not, depends on the contents. \$\endgroup\$ – Filcuk Jul 31 at 16:23
  • \$\begingroup\$ I adjusted the code accordingly. Thanks again, I shifted the Call break_lines line so it's above the Saveas line, and effectively removed the savechanges line, since you're correct to say it's redundant. After the links are broken there's just a handful of formulas/sheet so it's not likely to make a big difference, but if it can run even a bit faster why not. \$\endgroup\$ – Mark S. Jul 31 at 17:38

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.