Skip to main content
added 20 characters in body
Source Link
Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463

As a bonus, your macrothe TickerSheetUpdater class can be unit-tested because the dependencies are under control - and now if somebody cuts the network cable, you can handle errors gracefully, instead of having 200 UDF calls blowing up one after another.

As a bonus, your macro can be unit-tested because the dependencies are under control - and now if somebody cuts the network cable, you can handle errors gracefully, instead of having 200 UDF calls blowing up one after another.

As a bonus, the TickerSheetUpdater class can be unit-tested because the dependencies are under control - and now if somebody cuts the network cable, you can handle errors gracefully, instead of having 200 UDF calls blowing up one after another.

added 44 characters in body
Source Link
Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463

As a bonus, your codemacro can be unit-tested because the dependencies are under control - and now if somebody cuts the network cable, you can handle errors gracefully, instead of having 200 UDF calls blowing up one after another.

As a bonus, your code can be unit-tested - and now if somebody cuts the network cable, you can handle errors gracefully, instead of having 200 UDF calls blowing up one after another.

As a bonus, your macro can be unit-tested because the dependencies are under control - and now if somebody cuts the network cable, you can handle errors gracefully, instead of having 200 UDF calls blowing up one after another.

Source Link
Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463

Worksheet functions were not designed for this. User-defined worksheet functions were not made a feature of Excel to go and fetch data online - in my humble opinion, this is a terrible abuse of UDF's.

A function like this should be part of the definition of a class module that defines an object responsible for this task, and wrapped with an interface, say ITickerPriceProvider:

Public Function GetTickerPrice(ByVal ticker As String) As Double
End Function

Then your code moves into a class module with Implements ITickerPriceProvider, and the signature becomes this:

Option Explicit
Implements ITickerPriceProvider

Private Function ITickerPriceProvider_GetTickerPrice(ByVal ticker As String) As Double
    'your code
End Function

What gives? Now you can get fancy and write a macro that looks like this:

Public Sub UpdateTickerPrices()

    Dim provider As ITickerPriceProvider
    Set provider = New WebTickerPriceProvider
    
    Dim updater As TickerSheetUpdater
    Set updater = New TickerSheetUpdater

    updater.UpdateTickerPrices provider, TickerPricesSheet

End Sub

And the TickerSheetUpdater.UpdateTickerPrices method is where you go and implement the code that locates ticker strings (your worksheet must have a column with those, right?), fetches the prices for each one, and updates the worksheet.

This is a job for a macro, not a UDF.

As a bonus, your code can be unit-tested - and now if somebody cuts the network cable, you can handle errors gracefully, instead of having 200 UDF calls blowing up one after another.


As for the implementation itself, I don't see any glaring issues. I would have named IE something like browser, URL would have been url, and there would have been an On Error GoTo CleanFail statement at the top, and a CleanExit label to ensure resources get cleaned up whether or not an error occurs:

Private Function ITickerPriceProvider_GetTickerPrice(ByVal ticker As String) As Double
    On Error GoTo CleanFail

    'implementation here

CleanExit:
    Set browser = Nothing
    Set pageData = Nothing
    Exit Function

CleanFail:
    'handle errors
    Resume CleanExit

End Function

Turning ScreenUpdating off isn't going to help much, at least not with a UDF approach - the function isn't updating any cell values, it is a cell value.

However I'd try to see if there wouldn't be a way to remove this:

Application.Wait Now + TimeSerial(0, 0, 5)

...or add a comment that explains why it's needed, and why it needs to be a whole 5 seconds - especially given you've just waited for a specific ReadyState on your IE object:

 Do Until IE.ReadyState >= 4
        DoEvents
 Loop

What's 4 anyway? Make an Enum for the possible values, and use it instead of hard-coding a value like this. Why do you need to wait 5 seconds after the browser gets into that ReadyState? Could you keep loop-waiting for a later state value? An enum would help understanding what's going on and why here.