6
\$\begingroup\$

I have a spreadsheet where I control my shift hours. In such spreadsheet I input the date, the start time, time left for lunch, time back to work, end time and sometimes the overtime.

After that I have created a couple of functions to calculate the amount of time worked, the difference against the 8-hour shift expected time, bank of hours and so forth.

Everything is working fine, but I'm not very happy with the performance of it and I suspect the reason is one of my functions, which checks whether the current date is a holiday or not.

This is my current function:

Function IsHoliday(cell As Date) As Boolean
 Dim item As Range
 Dim holidayList As Range
' On Error Resume Next
 Set holidayList = Sheet2.Range("A3:A17")
 For Each item In holidayList
 If cell = item.Value Then
 IsHoliday = True
 Exit Function
 End If
 Next
 On Error GoTo 0
 IsHoliday = False
End Function

Sheet2 from the code is related to this spreadsheet:

Holiday Spreadsheet

The function is called in for each row in my Main Sheet, as highlighted in the picture below:

Main Spreadsheet

There are probably more issues in this, but I think the main issue is that I'm using a For Each loop to check if the date is a holiday in each row, but I sincerely don't know a faster way to do it. Can anyone share their thoughts on improvement points?

If you feel the need to check other functions and/or calculations in order to help me, just let me know.

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Oct 6, 2016 at 20:17
\$\endgroup\$
2
  • 3
    \$\begingroup\$ Since you already have a list of the holidays - why not just use a Match to that list? Avoid the UDF all together. \$\endgroup\$ Commented Oct 6, 2016 at 21:01
  • \$\begingroup\$ @Raystafarian I'm also calling the UDF in other function, so I feel it is necessary to have it, but thanks to your suggestion, I decided to use Match whenever I can in the spreadsheet. It seems to be helpful as well. \$\endgroup\$ Commented Oct 10, 2016 at 18:08

2 Answers 2

5
\$\begingroup\$

Since this is a UDF that clearly will get called quite a bit, you're right to be concerned with performance. Your search code can run MUCH faster if you perform the search on a memory array. That, along with a few other items noted here, will improve your function:

  1. Don't assume the workbook and worksheet. Your code is assuming it's running on the ActiveSheet when it doesn't have to be. So in your case, assign a couple variables to explicitly identify your execution environment. This guarantees far fewer headaches in the future.
  2. Dynamically size the range of your holiday list. Your function will be more flexible when you need to add holidays to work next year.
  3. Perform the search in a memory array. The biggest time consumer in your code is repeatedly having to read the data from a worksheet Cell in every loop each time the UDF is called. One single access to copy the data range to a memory array will drastically reduce your execution time.
  4. I find it easier to set a default return value for the function before I enter into a loop or a long stretch of logic. Then I only worry about changing the default return value if the tests pass.

Hopefully these tips will speed up your function and improve your code.

Option Explicit
Function IsHoliday(cell As Date) As Boolean
 Dim wb As Workbook
 Dim holidaySH As Worksheet
 Set wb = ThisWorkbook
 Set holidaySH = wb.Sheets("Sheet2")
 Const FIRSTROW = 3
 Dim lastRow As Long
 Dim holidayList As Range
 lastRow = holidaySH.Cells(holidaySH.Rows.Count, "A").End(xlUp).Row
 Set holidayList = holidaySH.Range("A" & FIRSTROW).Resize(lastRow - FIRSTROW, 1)
 Dim holidays As Variant
 holidays = holidayList
 Dim i As Long
 IsHoliday = False 'default return condition
 For i = 0 To UBound(holidays)
 If holidays(i) = cell.value Then
 IsHoliday = True
 Exit For
 End If
 Next i
End Function

EDIT: the code below should be quicker, in response to the suggestion from @MatsMug to use a static Dictionary.

Option Explicit
Public Function IsHoliday(cell As Range, _
 Optional forceReload as boolean = False) As Boolean
 '--- establish a static dictionary to populate once, then
 ' reuse with each call
 Const DATE_FORMAT = "mm/dd/yyyy"
 Static holidays As Scripting.Dictionary
 If (holidays Is Nothing) or forceReload Then
 Dim wb As Workbook
 Dim holidaySH As Worksheet
 Set wb = ThisWorkbook
 Set holidaySH = wb.Sheets("Sheet2")
 Const FIRSTROW = 3
 Dim lastRow As Long
 Dim holidayList As Range
 lastRow = holidaySH.Cells(holidaySH.Rows.Count, "A").End(xlUp).Row
 Set holidayList = holidaySH.Range("A" & FIRSTROW).Resize(lastRow - FIRSTROW, 1)
 Set holidays = New Scripting.Dictionary
 Dim i As Long
 For i = 1 To (lastRow - FIRSTROW)
 '--- the date (as a string) is the Key,
 ' the description as the Item
 holidays.Add Format(holidayList.Cells(i, 1), DATE_FORMAT), _
 holidayList.Cells(i, 2)
 Next i
 End If
 IsHoliday = False
 If holidays.Exists(Format(cell.value, DATE_FORMAT)) Then
 IsHoliday = True
 End If
End Function
answered Oct 7, 2016 at 15:22
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Nice answer! Holiday lookup time can be reduced from O(n) to O(1) with a Scripting.Dictionary - with some clever conditional initialization code the dictionary could be Static and reused between UDF calls, so it doesn't need to be populated everytime the function runs; the array would only ever be iterated once. \$\endgroup\$ Commented Oct 7, 2016 at 23:16
  • \$\begingroup\$ Thanks for the solution, I had to make a modification in the If holidays(i) = cell.value Then to If holidays(i,1) = cell Then because it is creating a 2D array and because cell is actually Date type. Other than that it seemed to improve the performance a little \$\endgroup\$ Commented Oct 10, 2016 at 18:05
  • \$\begingroup\$ @Mat'sMug, can you kindly elaborate on the Scripting.Dictionary? I feel it would also increase the performance with that, but I was not able to create a Static dictionary to be reused. \$\endgroup\$ Commented Oct 10, 2016 at 18:06
  • 1
    \$\begingroup\$ I'm hoping that I implemented the static dictionary in the edits above. \$\endgroup\$ Commented Oct 10, 2016 at 19:34
  • \$\begingroup\$ Thank you very much! With the Static dictionary in your edit I can really perceive the performance increase. That was really helpful \$\endgroup\$ Commented Oct 11, 2016 at 21:26
2
\$\begingroup\$

I'm not sure how quick this code would be due to using the Range rather than an array. In terms of lines of code it's definitely shorter.

If the value of cell isn't a holiday then the number of days between the single date is 1 day, if it's a holiday it will be 0 days.

Public Function IsHoliday(cell As Date) As Boolean
 Dim holidayList As Range
 Set holidayList = Sheet2.Range("A3:A17")
 IsHoliday = WorksheetFunction.NetworkDays(cell, cell, holidayList) = 0
End Function
answered Oct 26, 2016 at 13:55
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.