patternMinor
Speeding up execution time of worksheet-formatting code
Viewed 0 times
formattingtimecodeworksheetspeedingexecution
Problem
I've been writing some code that places some data in an empty worksheet and then formats the sheet by adding borders to all of the used cells. The code functions how it should perfect, but it takes a little bit of time to execute. I was wondering if there is any way I can speed up my code that I'm missing, because I don't know if there is a more efficient way to do what I've done.
```
Function UniqueRequest() As Long
'Creates dictionary full of unique request ID's and lists them in the "Request Results" sheet
Dim d As Object, tmp2
Dim LastRow As Long
Dim LastColumn As Long
Dim i As Long
Dim n As Long
Dim x As Long
Dim cllsct As Long
Dim myReqIDCol As Integer
Dim myNameCol As Integer
Dim myTypeCol As Integer
Dim myDescCol As Integer
Dim myVerCol As Integer
Dim myTstCol As Integer
'Determines the last row and the last column of the table
LastRow = Range("A" & Rows.count).End(xlUp).row
LastColumn = Cells(1, Columns.count).End(xlToLeft).Column
'Determines desired column number and stores it to a variable
myReqIDCol = ColSearch("id")
myNameCol = ColSearch("ownerFullname")
myTypeCol
Sub Report()
Application.ScreenUpdating = False
Sheets("Raw Data").Select
Application.Calculation = xlCalculationManual
Sheets("Raw Data").Unprotect
Req = UniqueRequest()
SheetForm = ReqSheetFormat()
ReqColor = ReqColorCount()
Sheets("Raw Data").Protect
Application.Calculation = xlCalculationAutomatic
Application.ScreenUpdating = True
End SubFunction ColSearch(Heading As String) As Integer
'Determines the column number of the desired heading
Sheets("Raw Data").Select
Sheets("Raw Data").Unprotect
Dim myCol As Integer
'Determines column number for desired string
myCol = Sheets("Raw Data").Cells.Find(What:=Heading, LookAt:=xlWhole, SearchOrder:=xlByRows, SearchDirection:=xlNext, MatchCase:=False).Column
ColSearch = myCol
End Function```
Function UniqueRequest() As Long
'Creates dictionary full of unique request ID's and lists them in the "Request Results" sheet
Dim d As Object, tmp2
Dim LastRow As Long
Dim LastColumn As Long
Dim i As Long
Dim n As Long
Dim x As Long
Dim cllsct As Long
Dim myReqIDCol As Integer
Dim myNameCol As Integer
Dim myTypeCol As Integer
Dim myDescCol As Integer
Dim myVerCol As Integer
Dim myTstCol As Integer
'Determines the last row and the last column of the table
LastRow = Range("A" & Rows.count).End(xlUp).row
LastColumn = Cells(1, Columns.count).End(xlToLeft).Column
'Determines desired column number and stores it to a variable
myReqIDCol = ColSearch("id")
myNameCol = ColSearch("ownerFullname")
myTypeCol
Solution
I only reviewed the first sub routine posted. I might get to the others at a later time.
Don't use PascalCase for variables, that should be for function, subs and classes. Single letter variables should be avoided. Also this is a personal preference but I like to put variable declarations right before to the initialization, especially with iterators for loops.
You request this sheet often.
I recommend declaring a
This should be done for any other sheets.
You create your header statically, first I recommend setting a header variable as you access it more than once.
Second, you should declare each field header as a constant and each column index as a constant.
Consider linking them in a function that returns a "Scripting.Dictionary" Object structured so that
Printing the header to the sheet should be a separate Sub routine.
Also I don't know why people keep using the
EDIT
See the ckuhn203's linked question about finding the last column and row. If your request sheets are guaranteed to be rectangular then your current method should be acceptable with prefixing
You could also use
Otherwise, use the linked solution. Put it into a separate function and keep it in a separate module for future use.
The redundancies in the next loop are staggering.
Making some those suggestions turns this to
There are 4 with statements like this that have nothing to do with
You could just move them out of the loop, but I would recommend also moving it to another function.
You also don't need to loop to add any borders. This will give thin borders to all cells in your used range.
So now your formatting is just
```
' Give al
Don't use PascalCase for variables, that should be for function, subs and classes. Single letter variables should be avoided. Also this is a personal preference but I like to put variable declarations right before to the initialization, especially with iterators for loops.
Dim i As Long
Dim j As Long
Dim LastRow As Long
Dim LastColumn As LongYou request this sheet often.
Sheets("Request Results")I recommend declaring a
WorkSheet variables and setting it to this sheet. Also consider declaring a Constant for the name of the sheet. If you use use this sheet with other macros consider making separate function just to access the sheet.Const REQUEST_SHEET_NAME As String = "Request Results"
Const REQUEST_BOOK_NAME As String = "your workbook's name"
Function GetRequestSheet() As WorkSheet
Set GetRequestSheet = Workbooks(REQUEST_BOOK_NAME).Sheets(REQUEST_SHEET_NAME)
End Function
' In Sub
Dim requestSheet as Worksheet
Set requestSheet = GetRequestSheet()This should be done for any other sheets.
You create your header statically, first I recommend setting a header variable as you access it more than once.
Dim headerRange as Range
Set headerRange = Range(requestSheet.Cells(1, 1), requestSheet.Cells(1, LastColumn))Second, you should declare each field header as a constant and each column index as a constant.
Consider linking them in a function that returns a "Scripting.Dictionary" Object structured so that
{column_index -> field_name}.Function GetHeaderDict() As Object
Set GetHeaderDict = CreateObject("Scripting.Dictionary")
GetHeaderDict.Add REQUEST_ID_COL, REQUEST_ID_HEADER
' ...
End FunctionPrinting the header to the sheet should be a separate Sub routine.
lastColumn = requestSheet.Cells(1, Columns.count).End(xlToLeft).Column
lastRow = requestSheet.Range("A" & Rows.count).End(xlUp).rowAlso I don't know why people keep using the
End method to count used rows and columns. I've used sheet.UsedRange.Rows.Count and sheet.UsedRange.Rows.Count. It considers data in all rows and columns not just the first. And I suspect it's faster, though I haven't tested it.EDIT
See the ckuhn203's linked question about finding the last column and row. If your request sheets are guaranteed to be rectangular then your current method should be acceptable with prefixing
Column.Count and Row.Count with your sheet.lastColumn = requestSheet.Cells(1, requestSheet.Columns.count).End(xlToLeft).Column
lastRow = requestSheet.Range("A" & requestSheet.Rows.count).End(xlUp).rowYou could also use
UsedRange if you are confident that "A1" is filled and you want to include cells that are used in any way.Otherwise, use the linked solution. Put it into a separate function and keep it in a separate module for future use.
The redundancies in the next loop are staggering.
- Declaring that
requestSheetObject will reduce clutter and prevent redundant access ofWorkSheetsjust to find the same sheet over and over again.
- Consider entering line breaks with the
_token to make this conditional more readable. This is a preference of mine. Others don't like to use_as much as I do.
- Declare constants for your colors and column Indexes, it makes the code easier to maintain and let's readers know why the cell/color is significant.
- maybe even declare a range object and set the cells to it
Making some those suggestions turns this to
Dim some_cell as Range
Set some_cell = requestSheet.Cells(i, SOME_COL)
Dim other_cell as Range
Set other_cell = requestSheet.Cells(i, OTHER_COL)
If some_cell.Value = "Yes" _
And other_cell.Value = "No" _
And other_cell.Interior.Color <> COLOR_BAD Then
other_cell.Interior.Color = COLOR_RED
ElseIf other_cell.Value = "Yes" Then
other_cell.Interior.Color = COLOR_MARK
ElseIf other_cell.Value = "No" _
And other_cell.Interior.Color <> COLOR_BAD Then
other_cell.Interior.Color = COLOR_MARK
End IfThere are 4 with statements like this that have nothing to do with
i and don't need to be looped over.With Sheets("Request Results").Range(Cells(1, 1), Cells(1, LastColumn))
.Borders(xlEdgeBottom).Color = 0
.Borders(xlEdgeBottom).Weight = xlThick
End WithYou could just move them out of the loop, but I would recommend also moving it to another function.
Sub GiveRangeBoldBorder(rng as Range)
Dim edge As Variant
For Each edge in Array(xlEdgeBottom, xlEdgeTop, xlEdgeLeft, xlEdgeRight)
With rng.Borders(edge)
.Color = 0
.Weight = xlThick
End With
Next edge
End SubYou also don't need to loop to add any borders. This will give thin borders to all cells in your used range.
With requestSheet.UsedRange.Borders
.Color = 0
.Weight = xlThin
End WithSo now your formatting is just
```
' Give al
Code Snippets
Dim i As Long
Dim j As Long
Dim LastRow As Long
Dim LastColumn As LongSheets("Request Results")Const REQUEST_SHEET_NAME As String = "Request Results"
Const REQUEST_BOOK_NAME As String = "your workbook's name"
Function GetRequestSheet() As WorkSheet
Set GetRequestSheet = Workbooks(REQUEST_BOOK_NAME).Sheets(REQUEST_SHEET_NAME)
End Function
' In Sub
Dim requestSheet as Worksheet
Set requestSheet = GetRequestSheet()Dim headerRange as Range
Set headerRange = Range(requestSheet.Cells(1, 1), requestSheet.Cells(1, LastColumn))Function GetHeaderDict() As Object
Set GetHeaderDict = CreateObject("Scripting.Dictionary")
GetHeaderDict.Add REQUEST_ID_COL, REQUEST_ID_HEADER
' ...
End FunctionContext
StackExchange Code Review Q#59717, answer score: 8
Revisions (0)
No revisions yet.