Please give some feedback on this implementation of Rock Paper Scissors

General
1

(Cross-posted from functional programming - "Rock, Paper, Scissors +" in F# - Code Review Stack Exchange)

During a discussion in a development chat, a user suggested to another (in the context of C# originally),

UserA: Challenge: Create an implementation of RPS, and then show how it can be extended to include an unlockable option Bomb (blows up paper and rock, but scissors cuts the fuse - basically Paper+; AI that has Bomb uses it instead of Paper) with a minimum of fuss.
UserA: Bonus: Make it so the game can be thus modified changing only data (constants, serialized fields, whatever).
UserB: how would one unlock the Bomb?
UserA: Basically “elsewhere” however you want. Could be an external property, could be an Interface or Serialized Field or parameter or whatever.

and since I’m interested in learning F# I decided to pick up the exercise. Along with that, I also tried to clean up the code for I/O handling, such that I could potentially swap a “player vs computer” game to “player vs player” or even “computer vs computer”. I have published the script in GitHub, but please find it pasted below too for convenience:

open System

type MatchResult =
    | Won
    | Lost
    | Draw

type Element =
    | Rock
    | Paper
    | Scissors
    | Bomb

type Player = {
    Pick: Element[] -> Element;
    AnnounceResult: MatchResult -> unit;
}

let victoriesOf element =
    match element with
    | Rock -> [Scissors]
    | Paper -> [Rock]
    | Scissors -> [Paper; Bomb]
    | Bomb -> [Rock; Paper]

let (!) (result: MatchResult) =
    match result with
    | Won -> Lost
    | Lost -> Won
    | x -> x

let evaluate player rival =
    let winsOver a b =
        victoriesOf a
        |> List.contains b
    match (winsOver player rival, winsOver rival player) with
    | (true, false) -> Won
    | (false, true) -> Lost
    | _ -> Draw

let parse options move =
    let cleanup (input: string) = input.ToLowerInvariant().Trim()
    let compare =
        string
        >> cleanup
        >> (=) move
    Seq.tryFind compare options

let runRound (a: Player) (b: Player) options =
    let aChoice = a.Pick options
    let bChoice = b.Pick options
    let result = evaluate aChoice bChoice
    a.AnnounceResult result
    b.AnnounceResult !result

let player = {
    Pick = fun options ->
        printfn "Choose one of %s:" (String.Join(", ", options))
        match parse options (Console.ReadLine()) with
        | Some(x) -> x
        | None -> failwith "Unknown choice!";
    AnnounceResult = fun result ->
        match result with
        | Won -> printfn "You won!"
        | Lost -> printfn "You lost!"
        | Draw -> printfn "It's a draw!"
}

let computer = 
    let rng = Random()
    {
        Pick = fun options -> 
            let choice = Array.item (rng.Next(options.Length)) options
            printfn "Computer chose %A" choice
            choice;
        AnnounceResult = fun _ -> ();
    }

[<EntryPoint>]
let main argv =
    let validMoves = [|Rock; Paper; Scissors; Bomb|]

    while true do
        runRound player computer validMoves
    0

Since I’m new to ML-like languages, I’m looking for feedback on how I could better use the F# libraries, better model my state and data, and even how to make it more point-free. And if there are more idiomatic ways of writing any piece of code, please tell!

2

Hello!

I think your solution here is pretty good! The only thing I would change would be your parse/pick methods for humans, which I would make a recursive function that didn’t fail with an exception. Something like:

Pick = fun options ->
        let rec chooser () = 
            printfn "Type one of %s:" (String.Join(", ", options))
            let choice = Console.ReadLine ()
            match Array.tryFind (fun o -> string o = choice) with
            | Some o -> o
            | _ -> 
                printfn "Invalid choice!"
                chooser ()
        chooser ()

Or something similar. Then you don’t have an ugly exception breaking up your flow. But thats a logic choice less a style choice. If you did go down this route, you could try creating a recursive function like the above, but making it tail call optimised. Or doing something fancy like Array.tryFind matching |> Option.defaultValue (chooser ())

In terms of point free style, which you shouldn’t go overboard on (“pointless style” as its derogatorily called sometimes), or just having lighter syntax in general one thing you could do more of is use function. For example, instead of:

AnnounceResult = fun result ->
        match result with
        | Won -> printfn "You won!"
        | Lost -> printfn "You lost!"
        | Draw -> printfn "It's a draw!"

you can do

AnnounceResult =
        function
        | Won -> printfn "You won!"
        | Lost -> printfn "You lost!"
        | Draw -> printfn "It's a draw!"

But this depends on whether you find it more or less readable.

Finally, in the theme of the original challenge, it might be more appropriate to have the options available as data, as in instead of a DU for rock, paper, scissors, bomb and a function to calculate victory, maybe have one of the ‘config’ inputs to the system be a map of string to string []. You would allow callers to easily configure the available options and what they beat, potentially from the command line or a config file, without requiring recompilation. But thats just how I would interpret the challenge.

Cheers, hope this is useful!

2 Likes
3

Thank you! And thanks for replying!

Great idea! I still need to practice not fearing tail calls.

That is an excellent description for overboard point-free-ness. I might borrow it from now on, xD.

I did not know about the function keyword, seems very useful, I kinda like it.

You are correct! I used a discriminated union to practice on its use a little bit, but now that it’s done I might move it to a soft configuration.

1 Like
4

Some thoughts regarding your code:

a) Style

  • “;” are unnecessary if you define fields on multiple line

  • usually pipelines look more naturally than brackets.

I.e.
String.Join(", ", options) |> printfn "Type one of %s:"

instead of
printfn "Type one of %s:" (String.Join(", ", options))

  • There is a build-in function ignore

  • To my opinion, it would be better to define not one-line function separately than directly in the record:

let userPick (options:Element[]) =
    ...  
let computerPick =
    ...
let announceToUser =
    ...
let player = { Pick = userPick; AnnounceResult = announceToUser }
let computer = { Pick = computerPick ; AnnounceResult = ignore }

Might be easier to perceive.

b) Pick isn’t a pure function (both for computer and a player). It also prints something to the console. Since messages aren’t for debug purpose I’d suggest moving them outside (to the runRound, for example)

c) AnnounceResult doesn’t look like a function that good fits to a Player type. It is the responsibility of a game system rather than a player.

1 Like