Copying columns and formats on condition

Question

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("H5").Value
 wsFinal.Range("AE2:AE" & lastRow).Value = Sheets(1).Range("H6").Value
 wsFinal.Range("AF2:AF" & lastRow).Value = Sheets(1).Range("H7").Value
 wsFinal.Range("AD2:AD" & lastRow).Value = Sheets(1).Range("H8").Value
 wsFinal.Range("F2:F" & lastRow).Value = Sheets(1).Range("H9").Value
 With wsImport.UsedRange
 Set importHeaderRng = .Rows(1) 'Import - Headers
 importLastRow = .Rows.Count 'Import - Total Rows
 End With
 With wsFinal.UsedRange
 finalHeaderRow = .Rows(1) 'Final - Headers (as Array)
 Set finalHeaderRng = .Rows(1) 'Final - Headers (as Range)
 End With
 With wsIndex.UsedRange 'Transpose col 3 from Index (without the header), as column names in Import
 Set indexHeaderCol = .Columns(3).Offset(1, 0).Resize(.Rows.Count - 1, 1)
 wsImport.Range(wsImport.Cells(1, 1), wsImport.Cells(1, .Rows.Count - 1)).Value2 = Application.Transpose(indexHeaderCol)
 End With
 If Len(bImport.Cells(1, 1).Value2) > 0 Then 'if Import sheet is not empty (1,1)
 With Application
 For Each header In finalHeaderRow 'Loop through all headers in Final
 If Len(Trim(header)) > 0 Then 'If the Final heade is not empty
 importHeaderFound = .Match(header, importHeaderRng, 0) 'Find header in Import sheet
 If IsError(importHeaderFound) Then
 msg = msg & vbLf & header & SPACE_DELIM & wsImport.Name 'Import doesn't have current header
 Else
 finalHeaderFound = .Match(header, finalHeaderRng, 0) 'Find header in Final sheet
 If IsError(finalHeaderFound) Then
 msg = msg & vbLf & header & SPACE_DELIM & wsFinal.Name 'Import doesn't have current header
 Else
 With wsImport
 Set importColRng = .UsedRange.Columns(importHeaderFound).Offset(1, 0).Resize(.UsedRange.Rows.Count - 1, 1)
 End With
 With wsFinal
 Set finalColRng = .Range(.Cells(2, finalHeaderFound), .Cells(importLastRow, finalHeaderFound))
 finalColRng.Value2 = vbNullString 'Delete previous values (entire column)
 End With
 finalColRng.Value2 = importColRng.Value2 'Copy Import data in Final columns
 End If
 End If
 End If
 Next header
 End With
 allUpper wsFinal
 Dim i As Long
 For i = 2 To lastRow
 If IsDate(wsFinal.Cells(i, "Q").Value) And Not IsEmpty(wsFinal.Cells(i, "Q").Value) Then
 With wsFinal
 .Cells(i, "Q").Value = Year(wsFinal.Cells(i, "Q").Value)
 End With
 End If
 Next i
 Dim j As Long
 For j = 2 To lastRow
 If IsDate(wsFinal.Cells(j, "R").Value) And Not IsEmpty(wsFinal.Cells(j, "R").Value) Then
 With wsFinal
 .Cells(j, "R").Value = Year(wsFinal.Cells(j, "R").Value)
 End With
 End If
 Next j
 wsFinal.Columns("G").NumberFormat = "MM/DD/YYYY"
 wsFinal.Columns("I").NumberFormat = "MM/DD/YYYY"
 wsFinal.Columns("A").NumberFormat = "@"
 wsFinal.Columns("B").NumberFormat = "@"
 wsFinal.Columns("C").NumberFormat = "@"
 wsFinal.Columns("R").NumberFormat = "@"
 wsFinal.Columns("Q").NumberFormat = "@"
 wsFinal.Columns("T").NumberFormat = "MM/DD/YYYY"
 wsFinal.Columns("W").NumberFormat = "MM/DD/YYYY"
 wsFinal.Columns("V").NumberFormat = "MM/DD/YYYY"
 wsFinal.Columns("AC").NumberFormat = "MM/DD/YYYY"
 wsFinal.Columns("N").NumberFormat = "_($* #,##0.00_);_($* (#,##0.00);_($* ""-""??_);_(@_)"
 wsFinal.Columns("AM").NumberFormat = "_($* #,##0.00_);_($* (#,##0.00);_($* ""-""??_);_(@_)"
 wsFinal.Columns("AN").NumberFormat = "_($* #,##0.00_);_($* (#,##0.00);_($* ""-""??_);_(@_)"
 wsFinal.Columns("AO").NumberFormat = "_($* #,##0.00_);_($* (#,##0.00);_($* ""-""??_);_(@_)"
 'wsFinal.UsedRange.AutoFilter
 applyFormat wsFinal.Range(wsFinal.Cells(1, 1), wsFinal.Cells(importLastRow, wsFinal.UsedRange.Columns.Count))
 Dim ws As Worksheet
 For Each ws In Worksheets
 ws.Select
 ActiveWindow.Zoom = 85
 Next ws
 Else
 MsgBox "Missing raw data (Sheet 2 - 'Import')", vbInformation, " Missing Raw Data"
 End If
End Sub
Private Sub applyFormat(ByRef rng As Range)
 With rng
 '.ClearFormats
 With .Font
 .Name = "Georgia"
 .Color = RGB(0, 0, 225)
 End With
 .Interior.Color = RGB(216, 228, 188)
 With .Rows(1)
 .Font.Bold = True
 .Interior.ColorIndex = xlAutomatic
 End With
 With .Borders
 .LineStyle = xlDot 'xlContinuous
 .ColorIndex = xlAutomatic
 .Weight = xlThin
 End With
 End With
 refit rng
End Sub
Private Sub allUpper(ByRef sh As Worksheet)
 Dim arr As Variant, i As Long, j As Long
 If WorksheetFunction.CountA(sh.UsedRange) > 0 Then
 arr = sh.UsedRange 'one interaction with the sheet
 For i = 2 To UBound(arr, 1) 'each "row"
 For j = 1 To UBound(arr, 2) 'each "col"
 arr(i, j) = UCase(RTrim(Replace(arr(i, j), Chr(10), vbNullString)))
 Next
 Next
 sh.UsedRange = arr 'second interaction with the sheet
 End If
End Sub
Public Sub allImportTrim()
 Dim arr As Variant, i As Long, j As Long, sh As Worksheet
 Set sh = bImport
 If WorksheetFunction.CountA(sh.UsedRange) > 0 Then
 arr = sh.UsedRange 'one interaction with the sheet
 For i = 2 To UBound(arr, 1) 'each "row"
 For j = 1 To UBound(arr, 2) 'each "col"
 arr(i, j) = RTrim(Replace(arr(i, j), Chr(10), vbNullString))
 Next
 Next
 sh.UsedRange = arr 'second interaction with the sheet
 End If
 refit sh.UsedRange
End Sub
Private Sub refit(ByRef rng As Range)
 With rng
 .WrapText = False
 .HorizontalAlignment = xlGeneral
 .VerticalAlignment = xlCenter
 '.Columns.AutoFit
 '.Rows.AutoFit
 .Columns.EntireColumn.AutoFit
 .Rows.EntireRow.AutoFit
 End With
End Sub

And a separate module that has minor functions and procedures:

Option Explicit
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
Sub transpose2()
 Sheets(2).Range("A1").EntireRow.Copy
 Sheets(1).Range("A2").PasteSpecial Transpose:=True
 Range("A1").ClearOutline
End Sub
 Sub FormatasDatex()
 'Format to Date for any headers that contain "DATE" on Sheet3
 Dim aCell As Range, bCell As Range
 Dim wsh As Worksheet
 Dim lastRow As Long, i As Long
 Dim ExitLoop As Boolean
 Set wsh = Worksheets(3)
 Set aCell = wsh.Rows(1).Find(What:="Date", LookIn:=xlValues, _
 Lookat:=xlPart, SearchOrder:=xlByRows, SearchDirection:=xlNext, MatchCase:=False, SearchFormat:=False)
 ExitLoop = False
 If Not aCell Is Nothing Then
 Set bCell = aCell
 wsh.Columns(aCell.Column).NumberFormat = "dd/mm/yyyy;@"
 lastRow = wsh.Range(Split(wsh.Cells(, aCell.Column).Address, "$")(1) & _
 wsh.Rows.Count).End(xlUp).Row
 For i = 2 To lastRow
 With wsh.Range(Split(wsh.Cells(, aCell.Column).Address, "$")(1) & i)
 .FormulaR1C1 = .Value
 End With
 Next i
 wsh.Columns(aCell.Column).AutoFit
 Do While ExitLoop = False
 Set aCell = wsh.Rows(1).FindNext(After:=aCell)
 If Not aCell Is Nothing Then
 If aCell.Address = bCell.Address Then Exit Do
 wsh.Columns(aCell.Column).NumberFormat = "mm/dd/yyyy;@"
 lastRow = wsh.Range(Split(wsh.Cells(, aCell.Column).Address, "$")(1) & _
 wsh.Rows.Count).End(xlUp).Row
 For i = 2 To lastRow
 wsh.Range(Split(wsh.Cells(, aCell.Column).Address, "$")(1) & i).FormulaR1C1 = _
 wsh.Range(Split(wsh.Cells(, aCell.Column).Address, "$")(1) & i).Value
 Next i
 Else
 ExitLoop = True
 End If
 Loop
 End If
 End Sub
Sub GetRaw()
 Dim wb As Workbook, wb2 As Workbook
 Dim ws As Worksheet
 Dim vFile As Variant
 'Set source workbook
 Set wb = ActiveWorkbook '<~~ You might want to use ThisWorkbook instead
 'Open the target workbook
 vFile = Application.GetOpenFilename("Excel-files,*.xlsx", _
 1, "Select One File To Open", , False)
 If vFile = False Then Exit Sub
 'Set targetworkbook
 Set wb2 = Workbooks.Open(vFile)
 wb2.Worksheets(1).Cells.Copy wb.Worksheets(2).Range("A1")
 wb2.Close SaveChanges:=False
End Sub
Public Sub xlSpeed(Optional ByVal optimize As Boolean = True)
 With Application
 If optimize Then
 If .Calculation <> xlCalculationManual Then .Calculation = xlCalculationManual
 Else
 If .Calculation <> xlCalculationAutomatic Then .Calculation = xlCalculationAutomatic
 End If
 If .DisplayAlerts <> Not optimize Then .DisplayAlerts = Not optimize
 'If .DisplayStatusBar <> Not optimize Then .DisplayStatusBar = Not optimize
 'If .EnableAnimations <> Not optimize Then .EnableAnimations = Not optimize
 If .EnableEvents <> Not optimize Then .EnableEvents = Not optimize
 If .ScreenUpdating <> Not optimize Then .ScreenUpdating = Not optimize
 End With
 xlSheetsSpeed , optimize
End Sub
Private Sub xlSheetsSpeed(Optional ByVal paramSheet As Worksheet, Optional ByVal optimize As Boolean = True)
 If paramSheet Is Nothing Then
 For Each paramSheet In Application.ActiveWorkbook.Sheets
 With paramSheet
 .DisplayPageBreaks = False
 .EnableCalculation = Not optimize
 '.EnableFormatConditionsCalculation = Not optimize
 '.EnablePivotTable = Not optimize
 End With
 Next
 Else
 With paramSheet
 .DisplayPageBreaks = False
 .EnableCalculation = Not optimize
 '.EnableFormatConditionsCalculation = Not optimize
 '.EnablePivotTable = Not optimize
 End With
 End If
End Sub
Public Sub xlResetSettings()
 With Application
 .Calculation = xlCalculationAutomatic
 .DisplayAlerts = True
 .DisplayStatusBar = True
 .EnableAnimations = False
 .EnableEvents = True
 .ScreenUpdating = True
 Dim sh As Worksheet
 For Each sh In Application.ActiveWorkbook.Sheets
 With sh
 .DisplayPageBreaks = False
 .EnableCalculation = True
 .EnableFormatConditionsCalculation = True
 .EnablePivotTable = True
 End With
 Next
 End With
End Sub

Does this follow best practices?

Answer 1

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.

• paul bica: Thanks for your time ! I'll have to take blame where the blame is due: most of your feedback is on MY bad behavior: 1. horizontal alignment is mine; I do it for faster visual references and maintainability, but I'll adapt to anything that improves performance 2. "aIndex" is the worksheet code name, similar to the default "Sheet1". It replaces the normal WorkSheets(1), and it's the only global I usually use 3. "complicated logic for a boolean check" - point taken 4. all comments are mine, to explain everything the code does for learning purposes. I usually use self-documenting variable names
• RubberDuck: I think you'd really benefit from the book I linked to. You could pick one up used for about 30ドル. It has lots of VB examples in it.

Answer 2

Naming.

Did you notice, every single identifier in the VBA standard library (or any other library you might reference, for that matter) uses PascalCase? Following standard conventions make your code easier to read, and therefore to maintain.

Keep camelCase for locals and parameters. YELLCASE or UPPER_SNAKE_CASE is a commonly used convention for constants, so I wouldn't change that.

I'm not sure what casing this is using:

Sub FormatasDatex()

CamelCase would be FormatAsDateX, but that "X" only makes me want to dig into the implementation to see what it stands for... and it turns out it just seems an arbitrary suffix that tells me nothing about the date format - actually, the date format is much less relevant than the fact that the procedure is an extremely specialized piece of code that formats some very specific cells on a very specific worksheet.

That's very, very frail: if a worksheet is ever inserted anywhere before the 3rd sheet, you have a bug.

I need to say something about this prefixing pattern:

Dim aCell As Range, bCell As Range

Set wsIndex = aIndex
Set wsImport = bImport
Set wsFinal = cFinal

aCell and bCell aren't any better than cell1 and cell2 (which are just as awfully meaningless) - if aCell is a search result, then why not call it searchResult? Prefixes a, b and c seem completely arbitrary - and if they denote a sequence of some sort, that sequence seems outside the scope of the procedure and as such, doesn't belong there.

---

DoSomething

Procedures do something. Their names should always start with a verb that describes that. If the only name you can think of is vague and doesn't really describe what's going on, then chances are that you're looking at a procedure that's doing too many things. But I'll get back to that.

A procedure called xlSpeed doesn't say anything about what it's doing. Looking at its implementation and usage makes me wonder why it's not just called ToggleWaitMode with an Optional wait As Boolean = True parameter. Seeing that DisplayPageBreaks = False has no toggle state, makes me wonder... do page breaks really make that much of a difference when ScreenUpdating is disabled?

You know how property getters have this Property Get keyword in them, and return the property value? Any procedure that starts with Get, that doesn't return anything, has a horrible name. I'm completely confused with what GetRaw is supposed to be doing. My only clue is this comment:

Dim wsImport As Worksheet 'Raw

...which only makes sense after I've seen that GetRaw procedure and went WTF over it.

---

This is overly verbose:

If optimize Then
 If .Calculation <> xlCalculationManual Then .Calculation = xlCalculationManual
Else
 If .Calculation <> xlCalculationAutomatic Then .Calculation = xlCalculationAutomatic
End If

How about this?

.Calculation = IIf(optimize, xlCalculationManual, xlCalculationAutomatic)

Avoid double negatives in conditionals:

If .ScreenUpdating <> Not optimize Then .ScreenUpdating = Not optimize

"If screen updating is not not then". What's the cost of reassigning the same value, versus that of accessing it twice? Get rid of that confusing wording, and Keep It Simple, Stupid:

.ScreenUpdating = Not optimize

---

Object Model Usage

The Microsoft Excel object model exposes a Worksheets and a Sheets collection - if your code can't deal with chart sheets, then stick to the Worksheets collection.

As far as best practices go, this isn't it. Your intention is to reference a very specific worksheet, with very specific content. Referring to worksheets by their index makes the whole thing fall apart, the minute a user decides he prefers having the worksheets sorted by name, and starts rearranging the order of things.

All worksheets have a Name property, but the user can change that as well. The single safest way to refer to a specific worksheet, is using its CodeName. And best of all, that code name happens to be a workbook-scoped object reference to the Worksheet object you're after.

If I understand correctly, you have a global-scope reference to ThisWorkbook.Worksheets("Final") with cFinal, that you copy to wsFinal, ...when the worksheet in question might have code name Sheet23. Why not give the special sheet code name FinalSheet, and ditch all the global variables and their copies?

Consistency.

What's the difference between this:

Set rngs = ThisWorkbook.Sheets(2).Cells

And that?

wsFinal.Range("D2:D" & lastRow).Value = Sheets(1).Range("H2").Value

Nevermind that you should be using the Worksheets collection here - my question is, why is it fully qualified in one place, and implicitly qualified in the other?

---

Parameterize.

Take this short little procedure as an example:

Sub transpose2()
 Sheets(2).Range("A1").EntireRow.Copy
 Sheets(1).Range("A2").PasteSpecial Transpose:=True
 Range("A1").ClearOutline
End Sub

Ignoring the fact that it's implicitly Public (why is it even in another module? who else is calling it?) and its poor name (what happened to transpose1?), ...and that Range("A1") is implicitly referring to the active sheet (danger!), ...what was I going to say here?

Ah, yes. Parameters. Consider:

Public Sub CopyPasteSpecialTranspose(ByVal source As Range, ByVal destination As Range)
 source.Copy
 destination.PasteSpecial Transpose:=True
 Application.CutCopyMode = False
End Sub

This procedure says what it does, and does what it says. Nothing more. Clearing outlines on the active sheet is a rather surprising side-effect of transposing a range... especially if it just so happens that the active sheet isn't one of the worksheets involved in the copy+paste operation.

By passing in source and destination parameters, you've now written reusable code.

---

S.R.P.

Single. Responsibility. Principle. Do one thing, do it well. MainProcess is way too long to be doing that. Extract methods, my friend. Break that monster down into pieces that fit a screen, and have a name that says exactly what's going on.

Pass your "dependencies" as parameters, avoid implicit references to active sheet (or to anything for that matter - be explicit!), and regroup methods logically - don't send a handful of procedures into another module just for the heck of it!

A code module should read and unfold like a story: at the top you have a high level of abstraction with abstract concepts (CopySourceData, FormatOutputSheet, etc.), followed by more and more specialized methods with a lower and lower level of abstraction, that culminate with the knowledge of all the specific range addresses and other implementation details: if you want to know what the code does, you read the top of the module; when you want to know how it does it, you read the bottom.

I'd recommend Robert C. Martin's Clean Code for more details about abstraction levels, and many, many other things. The code samples are Java, but the concepts are pretty much language-agnostic.

• paul bica: You guys are awesome! Very methodical and consistent - I'm sure it's a reflection of your work. You made some excellent points (some of them are for me, but not all). I have the file he's working on and went over this entire functionality to fix some bugs, and improve performance - it's now quite decent at about 4 seconds for 50K records (down from 20-30 sec), but I was in a bit of a hurry. I'll post the code bellow as an answer - feel free to highlight any improvements (I'll also get the book) but for now I'll take anything I can get
• Mathieu Guindon: @paulbica thanks! make sure your answer actually reviews OP's code (code-dump answers are frowned upon on this site) - if you pretty much rewrote it and would like it reviewed, you can always post your own question (I believe the rags-to-riches community tag might be applicable there)
• paul bica: Thanks - I was almost ready to post the full code when I saw your reply. I'll start another thread for it

Answer 3

All points made by the previous reviews are great.

I'd emphasize the divide-and-conquer strategy of breaking up large functions into individual items designed for a distinct purpose. It will allow for reusability, reduced complexity, and ease of maintenance on the long run

Also, consistency in naming conventions

In reference to some of @Mat's points:

Set wsIndex = aIndex
Set wsImport = bImport
Set wsFinal = cFinal

CodeNames of the workbooks

.

• This is an excellent suggestion:

.Calculation = IIf(optimize, xlCalculationManual, xlCalculationAutomatic)

Avoid double negatives in conditionals

This line:

If .ScreenUpdating <> Not optimize Then .ScreenUpdating = Not optimize

started out as this:

.ScreenUpdating = Not optimize

and I changed it to avoid updating if it's not needed

• I'm stingy with names, like "msg". But CopySourceData, FormatOutputSheet is the self documenting cod I want to do (when I'm not pressured by deadlines)