Excel 2010 vba code - a cleaner code

Question

I have design the following code. I would like to understand if a named range can be used in the ws.cells(Y,2)? I have tried to name the code ws.Range("Name") but it failed. The intent is to search a column of data seeking out specific criteria (bold and <1). Once found, it populates the data results to another sheet. The search should be from top to bottom, until it finds the first 7 matches to the criteria. I am seeking assistance with writing the code so that is it 1) cleaner and 2) faster.

  X = 12
  Y = 4
  Z = 0

 Set ws = Worksheets("Schedule")

Do Until Z = 7
  If ws.Cells(Y, 2).font.Bold = True And ws.Cells(Y, 2) < 1 Then
      ws.Activate
      ws.Cells(Y, 2).Offset(rowOffset:=0, columnOffset:=1).Activate
      ActiveCell.Copy Destination:=Worksheets("Project Status").Cells(X, 3)
      ws.Cells(Y, 2).Offset(rowOffset:=0, columnOffset:=3).Activate
      ActiveCell.Copy Destination:=Worksheets("Project Status").Cells(X, 6)
      ws.Cells(Y, 2).Offset(rowOffset:=0, columnOffset:=4).Activate
      ActiveCell.Copy Destination:=Worksheets("Project Status").Cells(X, 7)
      ws.Cells(Y, 2).Offset(rowOffset:=0, columnOffset:=0).Activate
      ActiveCell.Copy Destination:=Worksheets("Project Status").Cells(X, 8)
      X = X + 1
      Y = Y + 1
      Z = Z + 1
  Else
    Y = Y + 1
  End If
Loop

Name Range is not a worksheet level range but a workbook level range so you don't have to `ws.range("name")` but simply `range("name")` — Rosetta, Aug 07 '16 at 05:51

Ralph · Answer 1 · 2016-08-08T15:03:23.213

The following code does not address "sub question" in respect to *named ranges" as I did not understand that part.

Yet, the following code is a bit shorter and maybe even easier to read. Also, some minor improvements were made in respect to speed:

Option Explicit

Public Sub tmpSO()

Dim WS As Worksheet
Dim X As Long, Y As Long, Z As Long

X = 12
Z = 0

Set WS = ThisWorkbook.Worksheets("Schedule")

With Worksheets("Project Status")
    For Y = 4 To WS.Cells(WS.Rows.Count, 2).End(xlUp).Row
        If WS.Cells(Y, 2).Font.Bold And WS.Cells(Y, 2).Value2 < 1 Then
            WS.Cells(Y, 2).Offset(0, 1).Copy Destination:=.Cells(X, 3)
            WS.Cells(Y, 2).Offset(0, 3).Copy Destination:=.Cells(X, 6)
            WS.Cells(Y, 2).Offset(0, 4).Copy Destination:=.Cells(X, 7)
            WS.Cells(Y, 2).Offset(0, 0).Copy Destination:=.Cells(X, 8)
            X = X + 1
            Z = Z + 1
'        Else
'            Y = Y + 1
        End If
        If Z = 7 Then Exit For
    Next Y
End With

End Sub

Maybe you can elaborate a bit more why you want to use named ranges and what you wish to achieve with them that you cannot achieve with the above code as is.

Update:

Miqi180 made me aware that there might be a performance difference when avoiding Offset by directly referencing the cells instead. So, I staged a small performance test on my system (Office 2016, 64-bit) to test this assumption. Apparently, there is a major performance difference of ~14% (comparing the average of 10 iterations using Offset and another 10 iterations avoiding it).

This is the code I used to test the speed difference. Please do let me know if you believe that this setup is flawed:

Option Explicit

' Test whether you are using the 64-bit version of Office.
#If Win64 Then
    Declare PtrSafe Function getTickCount Lib "kernel32" Alias "QueryPerformanceCounter" (cyTickCount As Currency) As Long
#Else
    Declare Function getTickCount Lib "kernel32" Alias "QueryPerformanceCounter" (cyTickCount As Currency) As Long
#End If

Public Sub SpeedTestDirect()

Dim i As Long
Dim ws As Worksheet
Dim dttStart As Date
Dim startTime As Currency, endTime As Currency

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
Application.EnableEvents = False

Set ws = ThisWorkbook.Worksheets(1)
ws.Cells.Delete

dttStart = Now
getTickCount startTime

For i = 1 To 1000000
    ws.Cells(i, 1).Value2 = 1
    ws.Cells(i, 2).Value2 = 1
    ws.Cells(i, 3).Value2 = 1
    ws.Cells(i, 4).Value2 = 1
    ws.Cells(i, 5).Value2 = 1
    ws.Cells(i, 6).Value2 = 1
Next i

Application.EnableEvents = True
Application.Calculation = xlCalculationAutomatic
Application.ScreenUpdating = True

getTickCount endTime
Debug.Print "Runtime: " & endTime - startTime, Format(Now - dttStart, "hh:mm:ss")


End Sub

Public Sub SpeedTestUsingOffset()

Dim i As Long
Dim ws As Worksheet
Dim dttStart As Date
Dim startTime As Currency, endTime As Currency

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
Application.EnableEvents = False

Set ws = ThisWorkbook.Worksheets(1)
ws.Cells.Delete

dttStart = Now
getTickCount startTime

For i = 1 To 1000000
    ws.Cells(i, 1).Offset(0, 0).Value2 = 1
    ws.Cells(i, 1).Offset(0, 1).Value2 = 1
    ws.Cells(i, 1).Offset(0, 2).Value2 = 1
    ws.Cells(i, 1).Offset(0, 3).Value2 = 1
    ws.Cells(i, 1).Offset(0, 4).Value2 = 1
    ws.Cells(i, 1).Offset(0, 5).Value2 = 1
Next i

Application.EnableEvents = True
Application.Calculation = xlCalculationAutomatic
Application.ScreenUpdating = True

getTickCount endTime
Debug.Print "Runtime: " & endTime - startTime, Format(Now - dttStart, "hh:mm:ss")

End Sub

Based on this finding the improved code should be (thanks to Miqi180):

Public Sub tmpSO()

Dim WS As Worksheet
Dim X As Long, Y As Long, Z As Long

X = 12
Z = 0

Set WS = ThisWorkbook.Worksheets("Schedule")

With Worksheets("Project Status")
    For Y = 4 To WS.Cells(WS.Rows.Count, 2).End(xlUp).Row
        If WS.Cells(Y, 2).Font.Bold And WS.Cells(Y, 2).Value2 < 1 Then
            WS.Cells(Y, 3).Copy Destination:=.Cells(X, 3)
            WS.Cells(Y, 5).Copy Destination:=.Cells(X, 6)
            WS.Cells(Y, 6).Copy Destination:=.Cells(X, 7)
            WS.Cells(Y, 2).Copy Destination:=.Cells(X, 8)
            X = X + 1
            Z = Z + 1
'        Else
'            Y = Y + 1
        End If
        If Z = 7 Then Exit For
    Next Y
End With

End Sub

Yet, it should be noted that the speed can still be very much improved by moving over to (1) copying values only / directly using .Cells(X, 3).Value2 = WS.Cells(Y, 2).Value2 (for example) and (2) furthermore by using arrays instead.

_{Of course this does not include yet the standard suggestions such as Application.ScreenUpdating = False, Application.Calculation = xlCalculationManual, and Application.EnableEvents = False.}

I agree with most of your suggested code cleanup, but why use `.Offset(0, 1)` etc. when you can just set the reference directly? This should work just as well and be slightly faster: `WS.Cells(Y, 3).Copy Destination:=.Cells(X, 3)` etc. — Miqi180, Aug 06 '16 at 21:37
@Miqi180: normally, I try to stick to the original code as much as possible whenever I provide an answer. I do that for two reasons: (1) simply to ensure that the OP can understand / follow the solution and can maintain / improve the code thereafter. (2) I don't want to impose my coding style on someone else. So, if I don't see a difference in performance or stability then I prefer to keep the OP code. Yet, it seems you are right and there is a performance difference. I changed the answer accordingly. Good catch! — Ralph, Aug 08 '16 at 14:51
Thx. Your basic methodology for testing the two cases is sound, since you only need to test whether `offset` slows things down as expected, or not. Some pointers: VBA has a built-in timer function called... `Timer`, which returns fractional portions of a second on Windows machines. (On Mac the resolution is 1 second). By dimensioning `startTime` and `endTime` as `Single` and setting them equal to `Timer`, the `Debug.Print` statements return the number of seconds (to two decimal places) elapsed, which seems more intuitive. Also, this way you don't need to declare the getTickCount function. — Miqi180, Aug 08 '16 at 22:16
@Miqi180 you are probably right. I was just coming from way back when this post was still applicable: http://stackoverflow.com/questions/3162826/fastest-timing-resolution-system According to this Microsoft recommends the use of GetTickCount. Also, (back then) Timer was deemed not as accurate / fast as GetTickCount. Some more background: https://randomascii.wordpress.com/2013/05/09/timegettime-versus-gettickcount/ But I guess I'll be switching over to `Timer` now. Thanks. — Ralph, Aug 08 '16 at 22:40
Just a suggestion of course, but It just makes writing performance testing code easier, and I think it's quite safe to use `Timer` in this case, even if it is slightly less accurate than other available timing functions, since we're only dealing with two time measurements before/after one million iterations. If the performance difference between the two cases was so small that more fine-grained timing functions were required, I would just use whichever code was easier for me to read ;) — Miqi180, Aug 08 '16 at 23:14
All, thanks greatly for the suggestions and ideas. Early it was asked why I want to use a range name. In the event I have to add columns or move the origination or destination cell(s), I do not desire having to update the code to reflect the new locations. It is my thought that ranges would allow me to move and the references would all be the same (re - the range names). — user6686605, Aug 12 '16 at 17:19

score 1 · Answer 2 · answered Aug 07 '16 at 05:59

Name range is a workbook level range, not a worksheet level range.

If the name range refers to the active sheet, then ws.range("name") will work. But if it refers to a non-active sheet, ws.range("name") will throw an error.

Because name range is a workbook level range, so you can simply do Range("name"). Then you'll not get the error above.

P/S: another way to write Range("Name") is [Name] which looks cleaner but missing the intellisense.

Excel 2010 vba code - a cleaner code

2 Answers2

Update: