# Sluggish Performance when using ranges from two workbooks to update the one

I'm hoping someone can help me here, I'm working with a file which has about 38000 rows and 180 columns. I'm using a macro to update the fields in the one workbook with the values in the other workbook, but it takes about 2 minutes to run. I'm looking for a way to reduce this time, I've tried everything I could find on previous questions but it's still too long.

As you can see in the code below, the macro checks to see the # of rows are the same in each workbook (note the one has 1 more row, hence the lastrow temp having +1) and then I want to check if the field in the temp file is a certain colour, if not then change it if etc... I use this colour to keep track of the values that have changed from the raw file, as I don't want these values to be overwritten again once they have been changed once. I use ranges so that I don't need to access the worksheets the entire time as this increases the execution time. Any help will be appreciated.

Sub SavetoTemp()

StartTime = Timer
Set wb = ThisWorkbook
Set DT = wb.Worksheets("Data Table")

With Application
.Calculation = xlCalculationManual
.ScreenUpdating = False
.DisplayStatusBar = False
.EnableEvents = False
.ActiveSheet.DisplayPageBreaks = False
DT.DisplayPageBreaks = False
End With

Dim TempFile As Workbook
Dim TempSheet As Worksheet
Dim LastCol As Long
Dim colLetEnd As String

If wb.Worksheets("Steering Wheel").Range("M30").Value = "" Then
MsgBox "Please Select a Temporary File First"
Exit Sub
Else
Set TempFile = Workbooks.Open(Range("M30").Value)
Set TempSheet = TempFile.Worksheets(1)

Dim LastRowDT As Long
LastRowDT = DT.Cells(Rows.Count, "A").End(xlUp).row
LastCol = DT.Cells(1, Columns.Count).End(xlToLeft).Column

Dim LastRowTemp As Long
LastRowTemp = TempSheet.Cells(Rows.Count, "A").End(xlUp).row + 1

Dim tempCell As Range
Dim r As Long
Dim c As Long
Dim rngDT As String
Dim rngTemp As Range
Set rngTemp = TempSheet.UsedRange
rngDT = "A" & 3 & ":" & colLetEnd & LastRowDT

If (LastRowTemp = LastRowDT) Then
For Each cell In DT.Range(rngDT)
Set tempCell = rngTemp.Cells(cell.row - 1, cell.Column)
If Not tempCell.Interior.Color = RGB(188, 146, 49) Then
If IsNumeric(cell) And Not IsEmpty(cell) Then
If (Not cell = tempCell) Or (IsEmpty(tempCell)) Then
tempCell.Interior.Color = RGB(188, 146, 49)
tempCell = cell
End If
Else
If Not (StrComp(cell, tempCell, vbTextCompare) = 0) Then
tempCell.Interior.Color = RGB(188, 146, 49)
tempCell = cell
End If
End If
End If
Next cell

TempSheet.Cells.EntireColumn.AutoFit
TempFile.Save
TempFile.Close

MsgBox "All Records Saved to Temp File Successfully!"
wb.Worksheets("Steering Wheel").Activate
MinutesElapsed = Format((Timer - StartTime) / 86400, "hh:mm:ss")
wb.Worksheets("Steering Wheel").Range("E48").Value = MinutesElapsed
With Application
.Calculation = xlCalculationAutomatic
.ScreenUpdating = True
.DisplayStatusBar = True
.EnableEvents = True
End With
Else
With Application
.Calculation = xlCalculationAutomatic
.ScreenUpdating = True
.DisplayStatusBar = True
.EnableEvents = True
End With
Exit Sub
End If
End If
End Sub


Any improvement to the code to reduce execution time is my ultimate goal.

• The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. – Mast Oct 21 '19 at 9:18

I will start off with an advice on performance and then comment on a few more general topics.

## Performance

Your general performance problem is probably that you make too many sheet accesses. These are slow. Instead, you could read in all values in a range in one go. Used on a range containing more than a single cell, the Value property returns a Variant containing a 2-dim array of the values in the first area of the range.

You could simply read in the values of both ranges, do the comparison and write the value from the data sheet if the values do not agree and your flag colour is not set. Unfortunately, you cannot avoid the single cell accesses for writing because you save whether the value is to be updated via the cell colour.

## Unqualified Member Calls

Please note that every unqualified use of the Cells member does resolve to ActiveSheet.Cells. The same is true for Rows and Columns. For your code, this does not really make a difference since you use these methods to calculate things that do not depend on a specific worksheet. However, it looks like an error at first.

## Use of Default Members

Using default members to shorten the code, usually is good for readability. Any future maintainer of the code will thank you if you are more explicit about what you do. Thus, it would be helpful to replace the implicit default member calls in statements like tempCell = cell with the equivalent version tempCell.Value = cell.Value.

## Code Names for Sheet Access

You can change the code name of sheets in the workbook containing the code in the properties window for the sheet. These names allow you to access the sheet directly via this name without requiring the stringly-typed access via Worksheets. E.g. if you set the code name of the sheet named "Steering Wheel" to SteeringWheel, you can access cell M30 via SteeringWheel.Range("M30").

## Avoiding Magic Numbers

In general, using the same explicit number in several places inside a program is a mistake waiting to happen. When the value needs to be changed later, one has to find all occurrences in the code and replace them all. To avoid this, it makes sense to give the value a name and save it in one place, e.g. as a constant. Here, this applies to the colour used as marker colour.

## Named Ranges

Accessing data from a worksheet via its address means that whenever the layout changes, the code has to be adapted as well. This can be avoided using named ranges. If you introduced a named range TempFilePathRange for cell M30 scoped to the steering wheel worksheet, you could get the file path via SteeringWheel.Range("TempFilePathRange").Value, which does not only work after layout changes, but also conveys much better what it does.

## Single Responsibility Principle

For maintainability, it is generally a good idea to follow the single responsibility principle (SRP), which basically says that each method should be responsible for one and only one thing. The hard thing about it is to find a good definition of responsibility for the given situation.

You procedure definitely has a lot of responsibilities: Disabling and enabling all automatic updates, getting the temporary file path, opening and closing the temporary file, getting the data worksheet, extracting the ranges to compare and executing the comparison.

You could split this into several procedures and functions. The outermost could simply do the enabling and disabling of Excel features and call a private procedure doing the comparison in between. The inner one could call a function that returns a reference to the temporary file (and Nothing if it does not exist), which calls another function responsible for determining the file name. Next, it could call a function that extracts the range used for comparison from the temporary file and one that returns the reference range. Then these could be sent to a procedure executing the actual comparisons and updates. Finally, the procedure would close the temporary file. Similarly, the comparison operation could also e extracted into its own function.

After such a split, each sub-procedure or function would do a short sequence of simple actions with the details hidden in the functions and procedures it calls. This is generally much easier to digest then having to deal with all details at once. In addition, this approach this basically documents you code automatically, provided you use meaningful names for the functions and procedures.