patternMinor
Copying columns and formats on condition
Viewed 0 times
conditioncolumnscopyingformatsand
Problem
I have made significant changes in the code and now it takes about 20~40 seconds to process each worksheet. This will conditionally copy column based on the column headers in sheet2 that match to the ones in sheet3.
Here is the main module:
```
Option Explicit
Public Sub projectionTemplateFormat()
Dim t1 As Double, t2 As Double
xlSpeed True
t1 = Timer
mainProcess
t2 = Timer
xlSpeed False
MsgBox "Duration: " & t2 - t1 & " seconds"
End Sub
Private Sub mainProcess()
Const SPACE_DELIM As String = " "
Dim wsIndex As Worksheet
Dim wsImport As Worksheet 'Raw
Dim wsFinal As Worksheet
Dim indexHeaderCol As Range
Dim msg As String
Dim importHeaderRng As Range
Dim importColRng As Range
Dim importHeaderFound As Variant
Dim importLastRow As Long
Dim finalHeaderRng As Range
Dim finalColRng As Range
Dim finalHeaderRow As Variant
Dim finalHeaderFound As Variant
Dim header As Variant 'Each item in the FOR loop
Dim lastRow As Long 'Manual Headers based on the number of rows in the raw data
Dim rngs As Range
Set wsIndex = aIndex 'This is the Code Name; top-left pane: aIndex (Index)
Set wsImport = bImport 'Direct reference to Code Name: bImport.Range("A1")
Set wsFinal = cFinal 'Reference using Sheets collection: ThisWorkbook.Worksheets("Final")
Set rngs = ThisWorkbook.Sheets(2).Cells
lastRow = rngs.Find(What:="*", After:=rngs.Cells(1), Lookat:=xlPart, LookIn:=xlFormulas, SearchOrder:=xlByRows, SearchDirection:=xlPrevious, MatchCase:=False).Row
wsFinal.Range("D2:D" & lastRow).Value = Sheets(1).Range("H2").Value
wsFinal.Range("AC2:AC" & lastRow).Value = Sheets(1).Range("H3").Value
wsFinal.Range("X2:X" & lastRow).Value = Sheets(1).Range("H4").Value
wsFinal.Range("Y2:Y" & lastRow).Value = Sheets(1).Range("
Here is the main module:
```
Option Explicit
Public Sub projectionTemplateFormat()
Dim t1 As Double, t2 As Double
xlSpeed True
t1 = Timer
mainProcess
t2 = Timer
xlSpeed False
MsgBox "Duration: " & t2 - t1 & " seconds"
End Sub
Private Sub mainProcess()
Const SPACE_DELIM As String = " "
Dim wsIndex As Worksheet
Dim wsImport As Worksheet 'Raw
Dim wsFinal As Worksheet
Dim indexHeaderCol As Range
Dim msg As String
Dim importHeaderRng As Range
Dim importColRng As Range
Dim importHeaderFound As Variant
Dim importLastRow As Long
Dim finalHeaderRng As Range
Dim finalColRng As Range
Dim finalHeaderRow As Variant
Dim finalHeaderFound As Variant
Dim header As Variant 'Each item in the FOR loop
Dim lastRow As Long 'Manual Headers based on the number of rows in the raw data
Dim rngs As Range
Set wsIndex = aIndex 'This is the Code Name; top-left pane: aIndex (Index)
Set wsImport = bImport 'Direct reference to Code Name: bImport.Range("A1")
Set wsFinal = cFinal 'Reference using Sheets collection: ThisWorkbook.Worksheets("Final")
Set rngs = ThisWorkbook.Sheets(2).Cells
lastRow = rngs.Find(What:="*", After:=rngs.Cells(1), Lookat:=xlPart, LookIn:=xlFormulas, SearchOrder:=xlByRows, SearchDirection:=xlPrevious, MatchCase:=False).Row
wsFinal.Range("D2:D" & lastRow).Value = Sheets(1).Range("H2").Value
wsFinal.Range("AC2:AC" & lastRow).Value = Sheets(1).Range("H3").Value
wsFinal.Range("X2:X" & lastRow).Value = Sheets(1).Range("H4").Value
wsFinal.Range("Y2:Y" & lastRow).Value = Sheets(1).Range("
Solution
This looks nice, but it's a proven waste of time.
Let's say I want to rename
And now I'm wasting precious time on formatting that block, you know, instead of writing code. The code below is no less readable once you get accustomed to it.
I do like your use of vertical white space though. It breaks your declaration up into logical chunks. There is a problem with that though. Declaring all of your variables at the top of the procedure adds space in between the declaration of a variable and it's use. It's proven that increasing the number of lines between a variable's declaration and it's end of life decreases code quality. I recommend you pick up a copy of Code Complete. Steve McConnell covers this is some detail.
So, try to declare your variables just before using them.
Once you get accustomed to this, you can begin to (at least partially) gauge the quality of a routine by looking at the distance between a variable's declaration and it's last use.
Speaking of, let's talk about that comment...
Where is
This....
This is reasonably complicated logic for a boolean check. Even though this is the only time may be using this logic, it is absolutely reasonable to extract it into a private boolean function; just to make the code readable.
Then back up in your
There's no reason to Activate/Select all over the place here. You use variable reference elsewhere, there's no reason not to do it here too.
Lastly, remove the commented out code. Commented out code is dead code, nothing but clutter. If you're afraid of losing it for some reason, then you're not using Version Control. Friend, you should be. You can do it the hard way, if you can't install any third party libraries, or take advantage the Source Control library in Rubberduck (Disclaimer, I'm one of the owners of the project). There is zero excuse not to be using some sort of source control.
This was by no means a thorough review. I didn't have a lot of time. Hopefully someone else comes by with a fine tooth comb.
Dim wsIndex As Worksheet
Dim wsImport As Worksheet 'Raw
Dim wsFinal As Worksheet
Dim indexHeaderCol As Range
Dim msg As String
Dim importHeaderRng As Range
Dim importColRng As Range
Dim importHeaderFound As Variant
Dim importLastRow As LongLet's say I want to rename
msg to be a little more verbose and less abbreviated. Then I get this.Dim wsIndex As Worksheet
Dim wsImport As Worksheet 'Raw
Dim wsFinal As Worksheet
Dim indexHeaderCol As Range
Dim message As String
Dim importHeaderRng As Range
Dim importColRng As Range
Dim importHeaderFound As Variant
Dim importLastRow As LongAnd now I'm wasting precious time on formatting that block, you know, instead of writing code. The code below is no less readable once you get accustomed to it.
Dim wsIndex As Worksheet
Dim wsImport As Worksheet 'Raw
Dim wsFinal As Worksheet
Dim indexHeaderCol As Range
Dim msg As String
Dim importHeaderRng As Range
Dim importColRng As Range
Dim importHeaderFound As Variant
Dim importLastRow As LongI do like your use of vertical white space though. It breaks your declaration up into logical chunks. There is a problem with that though. Declaring all of your variables at the top of the procedure adds space in between the declaration of a variable and it's use. It's proven that increasing the number of lines between a variable's declaration and it's end of life decreases code quality. I recommend you pick up a copy of Code Complete. Steve McConnell covers this is some detail.
So, try to declare your variables just before using them.
Dim wsIndex As Worksheet
Set wsIndex = aIndex 'This is the Code Name; top-left pane: aIndex (Index)Once you get accustomed to this, you can begin to (at least partially) gauge the quality of a routine by looking at the distance between a variable's declaration and it's last use.
Speaking of, let's talk about that comment...
Set wsIndex = aIndex 'This is the Code Name; top-left pane: aIndex (Index)Where is
aIndex defined? It's not defined in this Sub, which means you're using a global variable. Don't do that. Pass it in to the sub as an argument. Restrict it's scope. Global state is a nightmare once your code grows beyond anything trivial. Also, why not give these variables a reasonable name? If you need a comment to explain what a variable is, then you need to rename that variable.This....
If Len(bImport.Cells(1, 1).Value2) > 0 Then 'if Import sheet is not empty (1,1)This is reasonably complicated logic for a boolean check. Even though this is the only time may be using this logic, it is absolutely reasonable to extract it into a private boolean function; just to make the code readable.
Private Function IsSheetEmpty(ByVal ws As Worksheet) As Boolean
IsSheetEmpty = (Len(ws.Cells(1, 1).Value2) > 0)
End FunctionThen back up in your
If statement...If IsSheetEmpty(bImport) ThenThere's no reason to Activate/Select all over the place here. You use variable reference elsewhere, there's no reason not to do it here too.
Sub ClearAll()
Application.ScreenUpdating = False
Range("H2:H11").Select
Selection.ClearContents
Range("A2:A100").Select
Selection.ClearContents
Selection.ClearFormats
Sheets(2).Select
Cells.Select
Selection.ClearContents
ThisWorkbook.Sheets(3).Rows("2:" & Rows.Count).Delete
Sheets(1).Select
Range("A2").Select
ActiveSheet.UsedRange
ThisWorkbook.Save
Application.ScreenUpdating = True
End SubLastly, remove the commented out code. Commented out code is dead code, nothing but clutter. If you're afraid of losing it for some reason, then you're not using Version Control. Friend, you should be. You can do it the hard way, if you can't install any third party libraries, or take advantage the Source Control library in Rubberduck (Disclaimer, I'm one of the owners of the project). There is zero excuse not to be using some sort of source control.
This was by no means a thorough review. I didn't have a lot of time. Hopefully someone else comes by with a fine tooth comb.
Code Snippets
Dim wsIndex As Worksheet
Dim wsImport As Worksheet 'Raw
Dim wsFinal As Worksheet
Dim indexHeaderCol As Range
Dim msg As String
Dim importHeaderRng As Range
Dim importColRng As Range
Dim importHeaderFound As Variant
Dim importLastRow As LongDim wsIndex As Worksheet
Dim wsImport As Worksheet 'Raw
Dim wsFinal As Worksheet
Dim indexHeaderCol As Range
Dim message As String
Dim importHeaderRng As Range
Dim importColRng As Range
Dim importHeaderFound As Variant
Dim importLastRow As LongDim wsIndex As Worksheet
Dim wsImport As Worksheet 'Raw
Dim wsFinal As Worksheet
Dim indexHeaderCol As Range
Dim msg As String
Dim importHeaderRng As Range
Dim importColRng As Range
Dim importHeaderFound As Variant
Dim importLastRow As LongDim wsIndex As Worksheet
Set wsIndex = aIndex 'This is the Code Name; top-left pane: aIndex (Index)Set wsIndex = aIndex 'This is the Code Name; top-left pane: aIndex (Index)Context
StackExchange Code Review Q#93781, answer score: 6
Revisions (0)
No revisions yet.