6
\$\begingroup\$

This is my first F# program. It's a simple Rock-Paper-Scissors implementation.

I am looking for feedback of all sorts, e.g. the choice of types (e.g. List vs Array), whether there are parts that could be solved more elegantly / idiomatic, formatting, etc.

open System

let allowedActions = [ "r"; "p"; "s" ]

let randomItem list (rng: Random) =
    List.item (rng.Next(List.length list)) list

let computerAction () = randomItem allowedActions (Random())
let rec playerAction () =
    printfn "Write 'r' for Rock, 'p' for Paper, 's' for Scissors. Type 'q' to Quit."

    let input = Console.ReadLine()

    let isInputAllowed = List.contains input allowedActions || input = "q"
    match isInputAllowed with
    | true -> input
    | _ -> printfn "invalid input '%s'" input; playerAction()

let computerWin playerAction computerAction =
    printfn "The computer did win. '%s' beats '%s'." computerAction playerAction
let playerWin playerAction computerAction =
    printfn "The player did win. '%s' beats '%s'." playerAction computerAction
let tie anAction = printfn "It's a tie. '%s'" anAction

let ruling playerAction computerAction =
    match (playerAction, computerAction) with
    | ("r", "p")
    | ("p", "s")
    | ("s", "r") -> computerWin playerAction computerAction
    | ("r", "s")
    | ("p", "r")
    | ("s", "p") -> playerWin playerAction computerAction
    | (_, _) -> tie playerAction

let playGame playerAction computerAction =
    printfn "You've chosen '%s'." playerAction
    printfn "The computer chose '%s'." computerAction
    ruling playerAction computerAction
    ()

let game () =
    let playerAction = playerAction ()
    let computerAction = computerAction ()
    match playerAction with
    | "q" -> false
    | _ ->
        playGame playerAction computerAction
        true

[<EntryPoint>]
let rec main arg =
    printfn "Hello World to Rock Paper Scissors"
    match game () with
    | true -> main arg
    | false -> 0
```
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

As a first attempt it looks in many ways ok.

I have three "complaints":

a) You rely too much on string literals as identifiers for input. That is considered bad practice in - I think - every programming language and should be avoided because it's error prone (typos and illegal input etc.) hence not very robust.

Instead of "r", "p" and "s" you could use a discriminated union - which is a very important part of the F#-toolbox to be familiar with:

type Weapon =
    | Rock
    | Paper
    | Scissors

This will for instance make the match-statement in ruling much more readable and type safe:

let ruling playerWeapon computerWeapon =
    match (playerWeapon, computerWeapon) with
    | (Rock, Paper)
    | (Paper, Scissors)
    | (Scissors, Rock) -> computerWin playerWeapon computerWeapon
    | (Rock, Scissors)
    | (Paper, Rock)
    | (Scissors, Paper) -> playerWin playerWeapon computerWeapon
    | (_, _) -> tie playerWeapon

b) Don't repeat yourself. In the functions where you print the results, you do the same thing, and therefore you should extract the common parts to a generalized function, so that:

let computerWin playerAction computerAction =
    printfn "The computer did win. '%s' beats '%s'." computerAction playerAction
let playerWin playerAction computerAction =
    printfn "The player did win. '%s' beats '%s'." playerAction computerAction

changes to:

let showWinner name winnerWeapon looserWeapon = 
    printfn "The %s did win. '%A' beats '%A'." name winnerWeapon looserWeapon
let computerWin playerWeapon computerWeapon = showWinner "computer" computerWeapon playerWeapon
let playerWin playerWeapon computerWeapon = showWinner "player" playerWeapon computerWeapon

c) You recursively call the main function. I don't know if you actually violate any formal or informal rules but it just looks "ugly" to me. I would make a dedicated function that runs the game recursively.


FYI - I have below refactored your program while incorporating my suggestions above:

// HH: Instead of string literals you should use discriminated unions as identifiers for "weapons"
//     This is much more robust in respect to typos etc.
type Weapon =
    | Rock
    | Paper
    | Scissors
    static member Abbreviation w = (w.ToString().[0]).ToString().ToLower()
    static member ToWeapon ch =
        match ch with
        | "r" -> Rock
        | "p" -> Paper
        | "s" -> Scissors
        | _ -> failwith "Invalid Weapon char"

let weapons = [ Rock; Paper; Scissors ]

// HH: You should only instantiate a single random object - used throughout the session.
let rand = new Random()
let computerAction () = weapons.[rand.Next(weapons.Length)]


// HH: This now returns an optional value of None if the user wants to quit
//     or Some (Weapon) if a valid weapon is chosen
let rec playerAction () =
    let allowedActions = weapons |> List.map Weapon.Abbreviation
    let choices = weapons |> List.map (fun w -> sprintf "'%s' = %A" (Weapon.Abbreviation w) w)
    printfn "Enter:\n%s.\n'q' to Quit." (String.Join("\n", choices))
    let input = Console.ReadLine()

    let validWeapon w = List.contains w allowedActions
    match input with
    | "q" -> None
    | w when validWeapon w -> Some (Weapon.ToWeapon w)
    | _ -> printfn "invalid input '%s'" input; playerAction()

//HH: Never repeat yourself: extract a function to print the winner...
let showWinner name winnerWeapon looserWeapon = 
    printfn "The %s did win. '%A' beats '%A'." name winnerWeapon looserWeapon
// HH: ... and call that from the dedicated winner functions
let computerWin playerWeapon computerWeapon = showWinner "computer" computerWeapon playerWeapon
let playerWin playerWeapon computerWeapon = showWinner "player" playerWeapon computerWeapon
let tie anAction = printfn "It's a tie. '%A'" anAction

let ruling playerWeapon computerWeapon =
    // HH: By using discriminated unions this match
    //     expression is much more readble and robust
    match (playerWeapon, computerWeapon) with
    | (Rock, Paper)
    | (Paper, Scissors)
    | (Scissors, Rock) -> computerWin playerWeapon computerWeapon
    | (Rock, Scissors)
    | (Paper, Rock)
    | (Scissors, Paper) -> playerWin playerWeapon computerWeapon
    | (_, _) -> tie playerWeapon

let playGame playerWeapon computerWeapon =
    printfn "You've chosen '%A'." playerWeapon
    printfn "The computer chose '%A'." computerWeapon
    ruling playerWeapon computerWeapon
    ()

let runGame () =
    let playerAction = playerAction ()
    match playerAction with
    | Some playerWeapon ->
        let computerWeapon = computerAction ()
        playGame playerWeapon computerWeapon
        true
    | None -> false

// HH: Personally I don't like,that you call main recursively.
//     You probably don't violate any formal or informal rule, but it just look wrong to me
//     So make a dedicated function to start the game by
let rec play () =
    match runGame() with
    | true -> play()
    | false -> ()

I have changed some of the names so that they matches my naming. For the example I show that you can extent a discriminated union with members (static and instance). You could chose to have these methods as normal function instead.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much. I have learned already a lot and I will look up more of the patterns you've displayed and mentioned. \$\endgroup\$
    – cimnine
    Jul 22, 2020 at 11:58

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.