views:

140

answers:

6

What would be the most effective way to express the following code?

match cond.EvalBool() with
| true ->                
    match body.Eval() with
    | :? ControlFlowModifier as e ->
        match e with
        | Break(scope) -> e :> obj //Break is a DU element of ControlFlowModifier
        | _ -> next() //other members of CFM should call next()
    | _ -> next() //all other values should call next()
| false -> null

cond.EvalBool returns a boolean result where false should return null and true should either run the entire block again (its wrapped in a func called next) or if the special value of break is found, then the loop should exit and return the break value.

Is there any way to compress that block of code to something smaller?

+4  A: 

I think that the code that you have written is fine. Here's an alternative which I marginally prefer:

let isBreak = function | Break(_) -> true | _ -> false
if cond.EvalBool() then
  match body.Eval() with
  | :? ControlFlowModifier as e when isBreak e -> e :> obj
  | _ -> next()
else
  null
kvb
Well, you beat me to it :)
Kha
@Kha - it's eerie how similar our approaches are. In fact, I was also going to use `box e` instead of `e :> obj`, but decided that it was strictly a personal style thing...
kvb
+2  A: 

I'm not too fond of matching booleans instead of using if-else. What about

let isBreak = function Break _ -> true | _ -> false
...

if cond.EvalBool() then
    match body.Eval() with
    | :? ControlFlowModifier as e when isBreak e -> box e
    | _ -> next()
else null

Or, if you think that special isBreak function shouldn't be necessary (I'd understand that), lets try creating a more general function: C#'s as operator

let tryCast<'T> (o : obj) =
    match o with
    | :? 'T as x -> Some x
    | _ -> None
...

if cond.EvalBool() then
    match body.Eval() |> tryCast with
    | Some (Break _ as e) -> box e //Break is a DU element of ControlFlowModifier
    | _ -> next() //all other values should call next()
else null
Kha
+1  A: 

I ended up creating an active pattern for this. Similar logic exist elsewhere so I could make it reusable

let rec next() : obj =
if cond.EvalBool() then 
    match body.Eval() with
    | IsBreak(res) -> res
    | _ -> step.Eval() |> ignore ; next()
else null

Looks decent?

Roger Alsing
+3  A: 

I want to point out that it appears there's a subtype hierarchy for the result type of Eval, and if instead that were also a DU, then you could do something like

match body.Eval() with
| ControlFlowModifier(Break e) -> box e
| _ -> next()

Hurray for nested patterns.

Brian
Yes, nested patterns are awesome. Even more so when you start bringing in active patterns.
gradbot
Ummm it doesnt appear to work to put the DU type name first and the DU element inside it like that?Cant get it to compile
Roger Alsing
This assumes body.Eval() returns a value of type Foo, which is a DU that has a case "ControlFlowModifier of Bar", where Bar is also a DU that has case "Break of Scope" or whatever.
Brian
A: 

In case you mean "most effective way" as shortest code, i vote to AP too:

let (|CondEval|_|) (c,_) = if c.EvalBool() then Some true else None
let (|BodyEval|_|) (_,b) =
  match b.Eval() with
  | ControlFlowModifier as e -> Some e
  | _ -> None

match cond,body with
| CondEval _ & BodyEval e -> e :> obj
| true -> next()
| false -> null
ssp
+1  A: 

To flatten the nested match constructs, you'll need to use nested patterns. This works best for discriminated unions (as pointed out by Brian - and I agree that designing F# code to use primarily discriminated unions is the best thing you can do).

Otherwise, you'll need some active patterns if you want to write the code succinctly using match (ssp posted one example, which shows active patterns specifically for your problem). However, you can do this using the following two reusable active patterns:

let (|TryCast|_|) a : 'res option =    
  match (box a) with 
  | :? 'res as r -> Some(r)
  | _ -> None

let (|Value|) (l:Lazy<_>) = l.Value  

The first one is like :?, but it allows you to nest other patterns to match the value (which isn't possible with as). The second one forces evaluation of lazy value (I suppose that both of them could be declared in F# libraries as they are quite useful). Now you can write:

match lazy cond.EvalBool(), lazy body.Eval() with
| Value(true), Value(TryCast((Break(scope) : ControlFlowModifier)) as e) -> 
    e :> obj //Break is a DU element of ControlFlowModifier  
| Value(true), _ -> 
    next() //all other values should call next()  
| _, _ -> null 

EDIT: As Roger pointed out in a comment, this version of the code may not be very readable. I think a better option would be to use only TryCast and format your original code slightly differently (although this isn't completely standard indentation, it is correct and F# compiler handles it fine):

match cond.EvalBool() with 
| false -> null 
| true ->                 
match body.Eval() with 
| TryCast(Break(scope) as e) -> e :> obj
| _ -> next()

This is probably the most readable option based on pattern matching, but you could also use if instad of the first match as in the version by kvb and combine it with TryCast (this really depends on personal preferences):

if cond.EvalBool() then
  match body.Eval() with 
  | TryCast(Break(scope) as e) -> e :> obj
  | _ -> next()
else null

In any case, I believe that TryCast makes the code more readable as you avoid one nesting (which is othervise required because of :? .. as ..).

Tomas Petricek
While I do find this approach technically interesting, the readabillity of the code pretty much vanished (IMO).
Roger Alsing
@Roger: Yes, I actually think the original code was okay already. Maybe using only `TryCast` would be the best option.
Tomas Petricek