• Hi All

    Please note that at the Chandoo.org Forums there is Zero Tolerance to Spam

    Post Spam and you Will Be Deleted as a User

    Hui...

  • When starting a new post, to receive a quicker and more targeted answer, Please include a sample file in the initial post.

Can this code be optimized?

jbaich

Member
Hi all, below is the macro I've been working on, and it seems to work, but takes a long time... I had anticipated it would based on what I'm asking it to do with pdf files and the sheer size of the workbook, as well as all the formulae and calculations within the workbook. So I'm not trying to make it faster than it can be, just want to make sure I'm not making it slower than it should be with the code I've written/compiled from others much smarter than myself! :) Aslo, the variables that start with x and end with $... I found other code that used these and decided they might work for me, but I have no idea if they are ideal... they are unknowns to me.

Code:
Option Explicit

Sub CopyTemplate()

Dim LastRw As Long
Dim FileRef As String
Dim sh As Shape
Dim i As Long
Dim FldrRoot As String
Dim FldrLvl1 As String
Dim FldrLvl2 As String
Dim FldrLvl3 As String
Dim IPDF As Object
Dim Filename As String
Dim PDFRef As String
Dim SubjectRow As Long
Dim xDirect2015PVS$, xDirect2014PVS$, InitialFoldr$

'''// This section prompts the user to define the variables xDirect2015PVS$ and xDirect2014PVS$, which are folder locations that the macro will import PDF objects from at a later point\\\
'''// This is intended to reduce potential naming errors as users will be required to create and populate these folders in advance\\\

InitialFoldr$ = "H:\"
With Application.FileDialog(msoFileDialogFolderPicker)
.InitialFileName = Application.DefaultFilePath & "\"
.Title = "Please select the folder containing the PVS's for the Roll Year under Appeal"
.InitialFileName = InitialFoldr$
.Show
If .SelectedItems.Count <> 0 Then
xDirect2015PVS$ = .SelectedItems(1) & "\"
Else

End If
End With

InitialFoldr$ = "H:\"
With Application.FileDialog(msoFileDialogFolderPicker)
.InitialFileName = Application.DefaultFilePath & "\"
.Title = "Please select the folder containing the PVS's for the PREVIOUS Roll Year"
.InitialFileName = InitialFoldr$
.Show
If .SelectedItems.Count <> 0 Then
xDirect2014PVS$ = .SelectedItems(1) & "\"
Else

End If
End With

Application.ScreenUpdating = False '//Turn off screen updating, stops the screen flickering and generally speeds up the macro\\


    '//Location to save completed folders and files\\
FldrRoot = "H:\SF PAAB\Back up files\Macro test" '//Will be setting this variable to be defined by promptig user input\\

           
                       
           
LastRw = Sheets("DO NOT DELETE").Range("B" & Rows.Count).End(xlUp).Row '//find last used row in column B\\

For i = 2 To LastRw '//Loop through each row\\

    On Error Resume Next
    SubjectRow = Sheets("DO NOT DELETE").Range("A" & i).Value '//Couldn't get application.match to work so added helper column with match formula to reference the subject row in the equity sheet\\
    On Error GoTo 0
   

    If SubjectRow > 0 Then
   
    Sheets("Equity").Activate
    ActiveSheet.Range("A" & SubjectRow).Value = "Y" '//Put the Subject value Y in \\
    ActiveSheet.Rows(SubjectRow).Select
    Selection.Font.Bold = True      'applies formatting to subject row
        With Selection.Interior
                .Pattern = xlSolid
                .PatternColorIndex = xlAutomatic
                .ThemeColor = xlThemeColorDark1
                .TintAndShade = -0.149998474074526
                .PatternTintAndShade = 0
    End With
       
       
    End If

                                                           
    FileRef = Sheets("DO NOT DELETE").Range("B" & i).Text  '//FileRef is the Appeal Number, which is also the folder tree as a string...\\
                                       
        '//This part defines the folder names and order from the FileRef string...\\
       
    FldrLvl1 = Left(FileRef, 4) '//Extract first 4 characters from string\\
    FldrLvl2 = Mid(FileRef, 6, 2) '//Extract middle 2\\
    FldrLvl3 = Right(FileRef, 5) '//Extract last 5\\
   
        '//This part creates the folders and file structure\\
   
    On Error Resume Next '//if folder exists - igonore error and carry on\\
    MkDir (FldrRoot & "\" & FldrLvl1) '//Create Level 1 folder (eg; 2015)\\
    MkDir (FldrRoot & "\" & FldrLvl1 & "\" & FldrLvl2) '//Create Level 2 folder (eg; 14)\\
    MkDir (FldrRoot & "\" & FldrLvl1 & "\" & FldrLvl2 & "\" & FldrLvl3) '//Create Level 3 folder (eg; 00002)\\
   
    On Error GoTo 0 '//resume errors\\
 
    '// This next section searches two folders, for files matching variable "Filename:=xDirect2015PVS$ & PDFRef"
    '// When found it basically copies and pastes the first page of the PDF file, from each folder to each worksheet... 2015 and 2014 for example

'Probably need to put some error handling in here if the sheet name doesn't match, or get user to define sheet name variables...\\
    For Each sh In Sheets(Year(Date) - 1 & " PVS").Shapes '//Find any current PDF pictures in the sheet\\
        sh.Delete '//Delete them :-)\\
       
    Next sh
       
'Probably need to put some error handling in here if the sheet name doesn't match, or get user to define sheet name variables...\\
    For Each sh In Sheets(Year(Date) - 2 & " PVS").Shapes '//Find any current PDF pictures in the sheet\\
        sh.Delete '//Delete them :-)\\
       
                             
    Next sh
   
    PDFRef = Sheets("DO NOT DELETE").Range("K" & i).Text
                 
        '//Is this the most efficient way to do this?\\
    Set IPDF = Sheets(Year(Date) - 1 & " PVS").OLEObjects.Add(Filename:=xDirect2015PVS$ & PDFRef, Link:=False, DisplayAsIcon:=False) '//Insert PDF, see Explanations sheet\\
   
    With IPDF
        .Top = Sheets(Year(Date) - 1 & " PVS").Range("A1").Top '//Set the top to be cell A1\\
        .Left = Sheets(Year(Date) - 1 & " PVS").Range("A1").Left '//Set the left to be cell A1\\
    End With

    On Error Resume Next '//if file exists - igonore error and carry on\\
   
                   
    Set IPDF = Sheets(Year(Date) - 2 & " PVS").OLEObjects.Add(Filename:=xDirect2014PVS$ & PDFRef, Link:=False, DisplayAsIcon:=False) '//Insert PDF\\
   
    With IPDF
        .Top = Sheets(Year(Date) - 2 & " PVS").Range("A1").Top '//Set the top to be cell A1\\
        .Left = Sheets(Year(Date) - 2 & " PVS").Range("A1").Left '//Set the left to be cell A1\\
    End With

    On Error Resume Next '//if file exists - igonore error and carry on\\
   
  '// This next part saves a copy of the workbook in the appropriate folder and then clears some formatting before repeating the process for each record in the loop\\
                                           
    Filename = Sheets("DO NOT DELETE").Range("F" & i).Text & " Summary Template.xlsm" '//Define the file name from column F\\
    ActiveWorkbook.SaveCopyAs (FldrRoot & "\" & FldrLvl1 & "\" & FldrLvl2 & "\" & FldrLvl3 & "\" & Filename) '//Save copy of the workbook\\
    'NOTE !!! If the file already exists it will be overwritten !!!
  On Error GoTo 0 '//resume errors\\
   
    Sheets("Equity").Activate
    Sheets("Equity").Range("A" & SubjectRow).Value = "" '//remove the Y for your calculations\\
    Rows(SubjectRow).Select    '//clears formatting of subject row\\
    Selection.Font.Bold = False
    With Selection.Interior
        .Pattern = xlNone
        .TintAndShade = 0
        .PatternTintAndShade = 0
    End With

Next
Application.ScreenUpdating = True '//Turn screen updating back on\\

MsgBox ("Excel has finished!")
 
   
End Sub

I have not attached a copy of the workbook because it is huge (9MB) and 9 WS's and I'd have to do A LOT of sanitizing so I thought I'd throw the code up, maybe there's something obvious I could change to optimize it? Also, is there anything I could do to the workbook, like turn off all calculations so that it's more static through this process and then goes back to a dynamic state on next open or something? Maybe it's all the lookups, and array formulas in the worksheets that are contributing to the slowness... Two of the worksheets have lots of data, 10,000 rows or more and the other sheets perform various analysis and present this data in various ways... can't really do too much about the size of these besides deleting a few columns that are probably not necessary and possibly cutting out a few thousand rows... would doing that make much of a difference?

Thanks,
Joe
 
Joe

You may want to have a read of:
https://blogs.office.com/2009/03/12/excel-vba-performance-coding-best-practices/
and
http://www.cpearson.com/excel/optimize.htm

It is typical to disable calculation and screen updating early in a macro and then re-enable them at the end

Code:
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

' Your code here

Application.ScreenUpdating = True
Application.Calculation = xlCalculationAutomatic

Just be aware that opening and saving a file will force a recalculation for each file anyway
 
Hi Joe ,

The easiest way to understand where the program takes time is to have the following statement at various places within the code ; after the program has finished executing , see the Immediate window for the time outputs. The longest interval between two time outputs is the segment which needs to be optimized.

Debug.Print Time

Narayan
 
Wow, that is great idea Narayan! Thanks... Looks like my loops are taking about 20 seconds each time to complete, which is better than I thought. Would you have any suggestions around error handling? One area that I think is vulnerable is...

Code:
'Probably need to put some error handling in here if the sheet name doesn't match, or get user to define sheet name variables...\\
    For Each sh In Sheets(Year(Date) - 1 & " PVS").Shapes '//Find any current PDF pictures in the sheet\\
        sh.Delete '//Delete them :-)\\
      
    Next sh
      
'Probably need to put some error handling in here if the sheet name doesn't match, or get user to define sheet name variables...\\
    For Each sh In Sheets(Year(Date) - 2 & " PVS").Shapes '//Find any current PDF pictures in the sheet\\
        sh.Delete '//Delete them :-)\\

If the user did not name their sheets according to this naming convention (Year(Date) - 1 & " PVS") this would be a big problem...

If I set these 2 sheet names as variables, could excel check for matches at the start of the Macro and prompt the user to rename if necessary?

Thanks again for the great idea with debug.print!
 
Joe

You may want to have a read of:
https://blogs.office.com/2009/03/12/excel-vba-performance-coding-best-practices/
and
http://www.cpearson.com/excel/optimize.htm

It is typical to disable calculation and screen updating early in a macro and then re-enable them at the end

Code:
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

' Your code here

Application.ScreenUpdating = True
Application.Calculation = xlCalculationAutomatic

Just be aware that opening and saving a file will force a recalculation for each file anyway

I was hoping this method of changing to xlCalculationManual would prevent my formula references from breaking (example #REF) if I was going to replace a worksheet referenced in the formula with an updated sheet, of the same name, but unless I'm doing something wrong, it does not seem to work this way... Is it possible to "Freeze" all formulas, delete a sheet, import a new sheet, rename it the same as the deleted sheet and then "Unfreeze" the formulas so that they reference the newly imported worksheet? The sheet being replaced is pretty large for copy paste, just curious if there was a better way...

Thanks,
Joe
 
Hi Joe ,

Sure , the code could check to see whether a sheet exists with a name according to the naming convention ; one way would be to loop through all the worksheets in the workbook , and see if any name matches the set format :

Code:
Public Sub CheckSheetName()
          Dim Pattern1 As String, Pattern2 As String
          Dim Sheetcount1 As Integer, Sheetcount2 As Integer, Userresponse As Integer
          Dim sh As Worksheet
          Dim shp As Shape
         
          Pattern1 = Year(Date) - 1 & " PVS"
          Pattern2 = Year(Date) - 2 & " PVS"
         
          Sheetcount1 = 0
          Sheetcount2 = 0
         
          For Each sh In ThisWorkbook.Worksheets
              If sh.Name Like Pattern1 & "*" Then Sheetcount1 = Sheetcount1 + 1
              If sh.Name Like Pattern2 & "*" Then Sheetcount2 = Sheetcount2 + 1
          Next
         
          If Sheetcount1 * Sheetcount2 = 0 Then
              Userresponse = MsgBox("Named sheets do not exist in this workbook ! Please check and rename tabs if required", vbYesNo)
              If Userresponse <> vbNo Then Exit Sub
          End If
         
          On Error Resume Next
          For Each shp In Sheets(Pattern1).Shapes '//Find any current PDF pictures in the sheet\\
              shp.Delete '//Delete them :-)\\
          Next shp
     
          For Each shp In Sheets(Pattern2).Shapes '//Find any current PDF pictures in the sheet\\
              shp.Delete '//Delete them :-)\\
          Next shp
          On Error GoTo 0
End Sub
Narayan

P.S. When there is no sheet which matches the 2 patterns , the user is asked whether they wish to check the workbook and rename any tabs ; they can either select NO to continue with program execution or YES to exit. This is the reason for the On Error Resume Next statement , since if they select to continue , the worksheets will not be found and the For ... Next loop statement will generate errors.
 
Can I ask one more quick question about the above code? right after the macro clears out any existing shapes or .pdfs the next part of the code looks in a folder for a new pdf to place in the sheet...
Code:
    PDFRef = Sheets("DO NOT DELETE").Range("K" & i).Text
                

    Set IPDF = Sheets(Pattern1).OLEObjects.Add(Filename:=CYPVS & "\" &  PDFRef, Link:=False, DisplayAsIcon:=False) '//Insert PDF

    With IPDF
        .Top = Sheets(Pattern1).Range("A1").Top '//Set the top to be cell A1\\
        .Left = Sheets(Pattern1).Range("A1").Left '//Set the left to be cell A1\\
    End With


   '//I have tried moving this On Error Resume Next line all over the place in an
' attempt to stop the info dialogue box if no matching file was found for the "Set IPDF"
'command below...  Sometimes there will not be a corresponding file for this part and
' that's ok, if there's no pdf, I just want the macro to skip this part and continue on....\\
    
   On Error Resume Next  '(Resume after End With???)
Set IPDF = Sheets(Pattern2).OLEObjects.Add(Filename:=PYPVS & "\" &  PDFRef, Link:=False, DisplayAsIcon:=False) '//Insert PDF

    With IPDF
        .Top = Sheets(Pattern2).Range("A1").Top '//Set the top to be cell A1\\
        .Left = Sheets(Pattern2).Range("A1").Left '//Set the left to be cell A1\\
  
    End With  '(Resume from On Error???)

Basically I'm trying to figure out how to use On Error Resume or GoTo at "End With"... I have read the Pearson article on error handling at http://www.cpearson.com/excel/errorhandling.htm
and it seems like I should be able to say On Error GoTo Err1: as my first on error statement, and then just type Err1: after the End With statement, but that doesn't seem to prevent the info box telling me that the file path cannot be found... how can I tell excel to just keep going and stop giving me the info box?

Or can I put in a statement like...
Code:
If File (Filename:=PYPVS & "\" &  PDFRef) = Exists Then 'I have not been successful with this test...

Set IPDF = Sheets(Pattern2).OLEObjects.Add(Filename:=PYPVS & "\" &  PDFRef, Link:=False, DisplayAsIcon:=False) '//Insert PDF

    With IPDF
        .Top = Sheets(Pattern2).Range("A1").Top '//Set the top to be cell A1\\
        .Left = Sheets(Pattern2).Range("A1").Left '//Set the left to be cell A1\\
  
    End With 
 Else
End If

Thanks,
Joe
 
So I'm thinking another if statement is the way to go?...
If Dir(PYPVS & "\" & PDFRef) <> "" Then... Is this a good solution?

Code:
PDFRef = Sheets("DO NOT DELETE").Range("V" & i).Text
  
  If PDFRef <> "No PVS Found" Then
  
  Set IPDF = Sheets(Pattern1).OLEObjects.Add(Filename:=CYPVS & "\" & PDFRef, Link:=False, DisplayAsIcon:=False) '//Insert PDF, see Explanations sheet\\
  
  With IPDF
  .Top = Sheets(Pattern1).Range("A1").Top '//Set the top to be cell A1\\
  .Left = Sheets(Pattern1).Range("A1").Left '//Set the left to be cell A1\\
  End With

  On Error Resume Next '//if file exists - igonore error and carry on\\
  
  If Dir(PYPVS & "\" & PDFRef) <> "" Then
  
  Set IPDF = Sheets(Pattern2).OLEObjects.Add(Filename:=PYPVS & "\" & PDFRef, Link:=False, DisplayAsIcon:=False) '//Insert PDF, see Explanations sheet\\
  
  With IPDF
  .Top = Sheets(Pattern2).Range("A1").Top '//Set the top to be cell A1\\
  .Left = Sheets(Pattern2).Range("A1").Left '//Set the left to be cell A1\\
  End With
  
  Else
  End If
  
  Else
  End If
 
Hiya jbaich
Just on your question about 'freezing' formula, a grubby solution which I have used before is to find and replace all '=' for something unused like '~', make your changes and then replace them again at the end.
It is by no means fast, or perfect, but in some cases it may save you a LOT of time - usually in manual maintenance.

Kind regards
 
Back
Top