HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Copying columns and formats on condition

Submitted by: @import:stackexchange-codereview··
0
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("

Solution

This looks nice, but it's a proven waste of time.

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


Let'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 Long


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.

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


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.

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 Function


Then back up in your If statement...

If IsSheetEmpty(bImport) Then


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.

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 Sub


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.

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 Long
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 Long
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 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.