5
\$\begingroup\$

My original code is:

let (upPosition, downPosition, leftPosition, rightPosition) = 
    match myMove.MyDirection with
    | Direction.Up -> (GoUp y, y, GoLeft x, GoRight x)
    | Direction.Down-> (y, GoDown y, GoLeft x, GoRight x)
    | Direction.Left -> (GoUp y, GoDown y, GoLeft x, x)
    | Direction.Right -> (GoUp y, GoDown y, x, GoRight x)
    | _ -> (y, y, x, x)

Basically, there is an object on the 2D board. myMove.MyDirection dictates its next move, and x,y are its target position.

Once the object gets to x,y, it would cause some impacts on its front, left, right neighbours from its local rotational space. GoUp, ... , GoRight determines how far those neighbours could be.

So, ultimately, the code returns a "+" (cross) shape centred at x,y, so the minimum variable to describe this shape is (x, y, upPosition, downPosition, leftPosition, rightPosition).

The best solution I can think of to the previous code is the following. However, I feel there is more can be done to make it better. Is there any map function for tuple and is it costly?

let Reverse = function
    | Direction.Up -> Direction.Down
    | Direction.Down -> Direction.Up
    | Direction.Left -> Direction.Right
    | Direction.Right -> Direction.Left
    | _ -> raise (ArgumentException("Not Vaild Move!"))

let Go myDirection filter pos = 
    match myDirection, filter with
    | a,b when a=b -> pos
    | Direction.Up , _  -> GoUp pos
    | Direction.Down , _ -> GoDown pos
    | Direction.Left , _ -> GoLeft pos
    | Direction.Right , _ -> GoRight pos
    | _ -> pos

let (upPosition, downPosition, leftPosition, rightPosition) =
    let rev = Reverse myMove.MyDirection
    (Go Direction.Up rev y, Go Direction.Down rev y, Go Direction.Left rev x, Go Direction.Right rev x)
\$\endgroup\$
4
  • \$\begingroup\$ I have to say, I'm a bit puzzled by this code. Why are there 4 values to represent a position? And why does the original code call both GoLeft and GoRight when the input is Direction.Up? Can you comment it a bit? \$\endgroup\$ Sep 7, 2012 at 23:23
  • \$\begingroup\$ @TomasPetricek Updated, Hopefully it is now better explained \$\endgroup\$
    – colinfang
    Sep 7, 2012 at 23:45
  • \$\begingroup\$ Can you add the definition of your Direction type? Also, where is the myMove variable coming from (it's not defined in the code you have here). \$\endgroup\$
    – Jack P.
    Sep 8, 2012 at 15:46
  • \$\begingroup\$ @JackP. Direction is a Enum with 4 directions.... the only arguments for this code is myMove.MyDirection, x,y \$\endgroup\$
    – colinfang
    Sep 8, 2012 at 17:38

1 Answer 1

3
\$\begingroup\$

It's tough to review only part of the code, because there are often small improvements which could be made but they depend on knowing the structure of the entire program -- for example, why are you using a tuple to pass around the positions ((upPosition, downPosition, leftPosition, rightPosition))? You'd likely be better off using a record type to encapsulate those values, since you'd only have to pass one variable around (the record) and it also facilitates structural matching which'll allow you to clean up some of your other functions.

The Direction enum would be much better as a discriminated union type (see the code below). Even though enums can define certain values (e.g., Up, Down, Left, Right) they can also take on any integer value by casting the value to the enum type. Discriminated union types also allow you to define a fixed set of values, but the F# type system ensures that your variables can have only the defined values -- which in turn, keeps you from having to use the wildcard pattern (_) to check for error conditions in your match statements.

Here's my version of your code -- it's largely the same, but I've changed Direction into a union type and moved the Reverse function into a new Direction module.

type Direction =
    | Up
    | Down
    | Left
    | Right

[<RequireQualifiedAccess; CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
module Direction =
    /// Reverses a specified direction.
    [<CompiledName("Reverse")>]
    let rev = function
        | Direction.Up ->
            Direction.Down
        | Direction.Down ->
            Direction.Up
        | Direction.Left ->
            Direction.Right
        | Direction.Right ->
            Direction.Left


[<CompiledName("Go")>]
let go myDirection filter pos =
    match myDirection with
    | x when x = filter ->
        pos
    | Direction.Up ->
        GoUp pos
    | Direction.Down ->
        GoDown pos
    | Direction.Left ->
        GoLeft pos
    | Direction.Right ->
        GoRight pos

let (upPosition, downPosition, leftPosition, rightPosition) =
    match myMove.MyDirection with
    | Direction.Up ->
        (GoUp y, y, GoLeft x, GoRight x)
    | Direction.Down->
        (y, GoDown y, GoLeft x, GoRight x)
    | Direction.Left ->
        (GoUp y, GoDown y, GoLeft x, x)
    | Direction.Right ->
        (GoUp y, GoDown y, x, GoRight x)

let (upPosition, downPosition, leftPosition, rightPosition) =
    let rev = Direction.rev myMove.MyDirection
    (go Direction.Up rev y, go Direction.Down rev y, go Direction.Left rev x, go Direction.Right rev x)

EDIT: Answers to your questions:

  1. Normally, you can't have a module and a type with the same name within a certain namespace, but the [<CompilationRepresentation(...)>] attribute I used adds "Module" to the name of the compiled module type. So in this case, the Direction module would be compiled to a type called DirectionModule.
  2. You could make rev a member function of the Direction type. However, in F# it's easier (in general) to consume module-based functions instead of type members because it works much better with F#'s type inference mechanism. In our example, if you had an instance of Direction you wouldn't be able to call myDirection.Reverse() unless you declared myDirection as a variable of type Direction (or the F# compiler was able to figure it out another way); if you call the module-based function (Direction.rev myDirection) F# can easily infer the type of the myDirection variable so you don't need any explicit type annotations.

    If you're consuming this code from C#, then I'd write the code like this to get the best of both worlds:

    type Direction =
        | Up
        | Down
        | Left
        | Right
    
        member this.Reverse () =
            match this with
            | Direction.Up ->
                Direction.Down
            | Direction.Down ->
                Direction.Up
            | Direction.Left ->
                Direction.Right
            | Direction.Right ->
                Direction.Left
    
    [<RequireQualifiedAccess; CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
    module internal Direction =
        /// Reverses a specified direction.
        [<CompiledName("Reverse")>]
        let inline rev (dir : Direction) =
            dir.Reverse ()
    

    In this version of the code, I've defined Reverse as a member on the direction type (making it easy to consume from C#), but I also have the module-based function which is easier to consume from F#. Note the module-based function is now marked inline; as it only calls the Reverse() member it makes sense for the F# compiler to emit code which directly calls Reverse(). Another way to think about this is that within this version of the code, the module-based rev function is only useful for type-inference purposes (but it is useful if you're using it from within F#).

  3. [<CompiledName(...)>] is an attribute specific to F#; when you use it on a function, the F# compiler will compile the function so it has the name you've specified in the attribute. This is primarily used when you want to consume some F# code from C#; you can define your F# functions with idiomatic F# names (e.g., rev), but by applying this attribute the methods in the compiled assembly can have idiomatic C# names (e.g., Reverse) to make them easier to consume.

\$\endgroup\$
2
  • \$\begingroup\$ Thx for the reply, I have a few questions based on ur code :) (a) Is it valid to declare module the same name as an existing type ? I thought as they are both classes in .net when i access from c#, they need to be named differently. (b) why not make rev as a member function of Direction type? (c) What is [<CompiledName("...")>] and what's the purpose of it? \$\endgroup\$
    – colinfang
    Sep 10, 2012 at 9:34
  • \$\begingroup\$ Hi @colinfang -- I've edited my response to include answers to your questions. \$\endgroup\$
    – Jack P.
    Sep 10, 2012 at 14:50

Your Answer

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

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