2
\$\begingroup\$

I'm learning F# and have a couple of routines much of whose functionality looks common so I am looking to refactor them together.

Here are the routines (which for the record I lifted from elsewhere:

let Prices time ID (polling:float) = 
  let sync = System.Threading.SynchronizationContext.Current
  let obs = new Event<int>() 
  let raiseEvent (value:int) = sync.Post((fun _ -> obs.Trigger(value)), null)  
  let interval = TimeSpan.FromSeconds(polling)
  let rec loop nextTime= async { 
    // Generate next value (on the GUI thread)    
    do getPrices nextTime
      |> List.filter (fun price -> price.ID = ID)
      |> List.iter (fun price -> raiseEvent price.Price)                        
    // Wait some short time
    do! Async.Sleep(1000)
    // Continue looping
    do! loop (nextTime.Add(interval)) }
    loop time |> Async.Start
    obs.Publish

and

let Volumes time ID (polling:float) = 
  let sync = System.Threading.SynchronizationContext.Current
  let obs = new Event<VolumeDTO>() 
  let raiseEvent (value:VolumeDTO) = sync.Post((fun _ -> obs.Trigger(value)),  null)  
  let interval = TimeSpan.FromSeconds(polling)
  let rec loop nextTime= async { 
    // Generate next value (on the GUI thread)    
    do getVolumes nextTime
      |> List.filter (fun volume -> volume.ID = ID)
      |> List.iter (fun volume -> raiseEvent volume)                        
    // Wait some short time
    do! Async.Sleep(1000)
    // Continue looping
    do! loop (nextTime.Add(interval)) }
    loop time |> Async.Start 
    obs.Publish

The external function calls are hopefully reasonable self explanatory retrieving data from a db

let getPrices lastTime =
  createList<PriceDTO>((getPriceSql lastTime), priceReader)

let getVolumes lastTime =
  createList<VolumeDTO>((getVolumeSql lastTime), volumeReader)

The types are very simple, but I'm not sure I've got this 'right':

type IID =
  abstract member ID : int64

type PriceDTO(ID:int64, Price:int) = 
  interface IID with
    member this.ID = ID
  member x.ID = ID
  member x.Price = Price

type VolumeDTO(ID:int64, Amount:decimal, Price:int32) = 
  interface IID with
    member this.ID = ID
  member x.ID = ID
  member x.Amount = Amount
  member x.Price = Price

I've played around with something but I'm not at all there as yet:

let publish<'a> time ID (polling:float) dataRetriever = 
  let sync = System.Threading.SynchronizationContext.Current
  let obs = new Event<'a>() 
  let raiseEvent (value:'a) = sync.Post((fun _ -> obs.Trigger(value)), null)  
  let interval = TimeSpan.FromSeconds(polling)
  let rec loop (nextTime:DateTime)= async { 
    // Generate next value (on the GUI thread)    
    do dataRetriever nextTime
      |> List.filter (fun item -> item.ID = ID)
      |> List.iter (fun item -> raiseEvent item)                        
    // Wait some short time
    do! Async.Sleep(1000)
    // Continue looping
    do! loop (nextTime.Add(interval)) }
    loop time |> Async.Start 
    obs.Publish

I may well have not quite implemented this correctly. I'm obviously getting a list of prices or volumes filtering them and publishing them. In order to achieve the filtering I've created an interface, but I'm not really happy with this since I'd prefer to keep the DTOs as simple as possible. Intellisense is telling me that my dataRetriever function takes a DateTime and returns an 'a list, but I'm not sure I quite want that since I need to cast it to an IID interface for filtering and then back to the underlying DTO for publishing.

Is there a more 'functional' (or perhaps just better) way to do this?

\$\endgroup\$

migrated from stackoverflow.com Jun 26 '12 at 14:45

This question came from our site for professional and enthusiast programmers.

7
\$\begingroup\$

This is a bit difficult to answer, because you did not share code sample that fully type-checks (and so it is hard to make sure the answer is correct). However, I think you're very close. When refactoring two similar functions, you just need to abstract out the bits that differ.

In your case, there are two places:

  • One is the function that is used to retrieve the data (either getPrices or getVolumes)
  • Second is the projection that is used when triggering the event (either item or item.Price)

If you take these two operations as functions, then you can write the following:

open System

type IID = 
  abstract member ID : int64 

let Publish (dataRetriever:TimeSpan -> list<#IID>) valueSelector time ID (polling:float) =  
  let sync = System.Threading.SynchronizationContext.Current 
  let obs = new Event<int>()  
  let raiseEvent (value:int) = sync.Post((fun _ -> obs.Trigger(value)), null)   
  let interval = TimeSpan.FromSeconds(polling) 
  let rec loop nextTime= async {  
    // Generate next value (on the GUI thread)     
    dataRetriever nextTime 
    |> List.filter (fun item -> item.ID = ID) 
    |> List.iter (fun item -> raiseEvent (valueSelector item))                         
    // Wait some short time 
    do! Async.Sleep(1000) 
    // Continue looping 
    do! loop (nextTime.Add(interval)) } 
  loop time |> Async.Start 
  obs.Publish 

The type annotation TimeSpan -> list<#IID> is interesting. It says that the argument is a function that returns a list of some values that implement the IID interface - but it turns this type into a generic type argument that is then passed to the other function (valueSelector), so when you use the function, the compiler knows that the argument of valueSelector has the same type as the thing that you obtain from dataRetriever. The annotation allows you to write item.ID in the filtering function.

The two functions can now be defined as follows:

let Prices = Publish getPrices (fun price -> price.Price)
let Volumes = Publish getVolumes id
\$\endgroup\$
  • 1
    \$\begingroup\$ Tomas, this is fantastic. TVM. 1Q - with prices I am just publishing a series of prices for each id (a list/set of prices), but for volume I am publishing series of amounts at various prices (a list/set of price/amount pairs). I notice you have removed what I had declared as a generic and made it Event<int> rather than Event<'a>. Was this by design? I was rather hoping to "put in" the different data 'under' the generic hood. \$\endgroup\$ – Simon Woods Jun 26 '12 at 8:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.