Battleship Game

Question

Inspired by this question, I went on with the following version. My goal was to focus on pure functions, to avoid mutable variables and to be strictly functional.

I have ignored the computer player part - as it will not change the overall design.

An issue may be that an unchanged player survives from one game state to the next. That's maybe not a good idea, but will it work here because game states are not used anywhere else?

module BattleShipGame

open System

let boardSize = 10

type Direction = Horz | Vert
type ShipType = 
  | AircraftCarrier
  | BattleShip
  | Frigate
  | Submarine
  | Minesweeper

type Point(x : int, y : int) = 
  struct
    member this.X = x
    member this.Y = y
  end

type Ship(position : Point, shipType : ShipType, direction : Direction, hits : Point list) = 
  member this.Position = position
  member this.Type = shipType
  member this.Direction = direction
  member this.Hits = hits
  member this.Length =
    match this.Type with
    | AircraftCarrier -> 5
    | BattleShip -> 4
    | Frigate | Submarine -> 3
    | Minesweeper -> 2

  member this.IsFinished =
    this.Hits.Length >= this.Length

  member this.HitTest(pos : Point) =
    let offset, dirValid = if this.Direction = Horz then pos.X - this.Position.X, pos.Y = this.Position.Y else pos.Y - this.Position.Y, pos.X = this.Position.X

    if not dirValid then
      false
    else
      offset >= 0 && offset < this.Length

  static member Copy(ship: Ship, hit : Point) =
    Ship(ship.Position, ship.Type, ship.Direction, hit :: ship.Hits)

type Player(name : string, ships : Ship list, enemyHits : Point list) = 
  member this.Name = name
  member this.Ships = ships
  member this.EnemyHits = enemyHits;
  member this.IsFinished = (this.Ships |> List.tryFind (fun sh -> sh.IsFinished = false) ).IsNone

  member this.Print() =
    let shipHits = this.Ships |> List.collect (fun sh -> sh.Hits)

    printf "y\x"
    for x in 1 .. boardSize do
      printf " %d " x
    printfn ""

    for y in 1 .. boardSize do
      for x in 0 .. boardSize do
        let hit = shipHits |> List.tryFind (fun pt -> pt.X = x && pt.Y = y)
        if x = 0 && y > 0 then
          printf "%3d" y
        elif hit.IsSome then
          printf " X "
        else 
          let pt = this.EnemyHits |> List.tryFind (fun pt -> pt.X = x && pt.Y = y)
          if pt.IsSome then
            printf " 0 "
          else
            printf "   ";
      Console.WriteLine()


  static member Copy(player : Player, hittedShip : Ship option, hit : Point) =
    if  hittedShip.IsSome then
      let shipList = Ship.Copy(hittedShip.Value, hit) :: player.Ships |> List.where (fun sh -> sh <> hittedShip.Value)
      Player(player.Name, shipList, player.EnemyHits)
    else
      Player(player.Name, player.Ships, hit :: player.EnemyHits)

// The current state of the game
type GameState(currentPlayer : Player, otherPlayer : Player) =
  member this.CurrentPlayer = currentPlayer
  member this.OtherPlayer = otherPlayer
  member this.IsFinished = currentPlayer.IsFinished || otherPlayer.IsFinished

// Prompts for a point in the board coordinate system x = horz, y = vert
let getUserTarget() =
  let rec gut() = 
    Console.Write("Enter target [x,y]:")
    let input = Console.ReadLine().Split(',')
    if input.Length <> 2 then
      gut()
    else
      let mutable x = 0
      let mutable y = 0
      if Int32.TryParse(input.[0], &x) && Int32.TryParse(input.[1], &y) then
        if x < 1 || x > boardSize || y < 1 || y > boardSize then
          gut()
        else
          Point(x, y)
      else
        gut()
  gut()

// Prints name of the current player and the board of the other player (which is the current players view of state)
let printBoard (state : GameState) =
  Console.Clear()
  printfn "Current Player: %s:" state.CurrentPlayer.Name
  state.OtherPlayer.Print()

// The "main" function which is repeatedly called by Seq.unfold below in main
let getState (state : GameState) =  
  if state.IsFinished then
    printBoard state
    None
  else
    printBoard state
    let target = getUserTarget()
    let hittest = state.OtherPlayer.Ships |> List.tryFind (fun ship -> ship.HitTest target)
    if hittest.IsSome then
      Some((state, GameState(state.CurrentPlayer, Player.Copy(state.OtherPlayer, hittest, target))))
    else
      Some((state, GameState(Player.Copy(state.OtherPlayer, None, target), state.CurrentPlayer)))

let initPlayer name = 
  // TODO: change to user input loop including all ship types
  if name = "Hillary" then
    let ships = [
      Ship(Point(1, 1), AircraftCarrier, Horz, []);
      Ship(Point(5, 3), BattleShip, Vert, []);
      Ship(Point(8, 9), Frigate, Horz, [])
    ]
    Player(name, ships, [])
  else
    let ships = [
      Ship(Point(1, 1), AircraftCarrier, Horz, []);
      Ship(Point(5, 3), BattleShip, Vert, []);
      Ship(Point(8, 9), Frigate, Horz, [])
    ]
    Player(name, ships, [])


[<EntryPoint>]
let main argv = 

  // Creates a sequence of game states of which the last holds the winner as the CurrentPlayer
  let result = Seq.unfold (fun state -> getState state) (GameState(initPlayer("Hillary"), initPlayer("Donald")))
  result
  |> Seq.last 
  |> fun state -> (printfn "\nWinner: %s" state.CurrentPlayer.Name)

  Console.WriteLine("END")
  Console.ReadLine() |> ignore
  0 

Answer 1

Disclaimer: I haven't tried compiling any of the code changes I've suggested, so if you try something and it doesn't work please let me know and I'll correct it

---

type Direction = Horz | Vert
type ShipType = 
  | AircraftCarrier
  | BattleShip
  | Frigate
  | Submarine
  | Minesweeper

I would not encode ship types as a union type. Generally, union types should imply some kind of different behaviour, in the same way that classes do in OOP. Instead, each ship could be an instance of a record type, with Length and Name. This means you can more easily add and remove ships. Also, just a personal preference but I'd use the full words for Directions, for readability's sake.

---

type Point(x : int, y : int) = 
  struct
    member this.X = x
    member this.Y = y
  end

I would use a record type here - you don't gain much by using a struct. As I touched on in this answer, you can move some of the coordinate-based logic to the Point type.

---

type Ship(position : Point, shipType : ShipType, direction : Direction, hits : Point list) = 
  member this.Position = position
  member this.Type = shipType
  member this.Direction = direction
  member this.Hits = hits
  member this.Length =
    match this.Type with
    | AircraftCarrier -> 5
    | BattleShip -> 4
    | Frigate | Submarine -> 3
    | Minesweeper -> 2

This may be a personal preference but for this type of data structure I'd use a record rather than a class:

type Ship = {
    Position: Point
    ShipType: ShipType
    Direction: Direction
    Hits: Point list
} with
    member this.Length =
        match this.ShipType with
        | AircraftCarrier -> 5
        | BattleShip -> 4
        | Frigate | Submarine -> 3
        | Minesweeper -> 2

You get copying for free (using the with syntax) as well as being generally more concise.

---

member this.IsFinished =
    this.Hits.Length >= this.Length

member this.HitTest(pos : Point) =
    let offset, dirValid = if this.Direction = Horz then pos.X - this.Position.X, pos.Y = this.Position.Y else pos.Y - this.Position.Y, pos.X = this.Position.X

    if not dirValid then
        false
    else
        offset >= 0 && offset < this.Length

You could write these function several ways and I guess it doesn't matter exactly how they're implemented, but personally I'd do something like:

member this.Points =
    let add (dir: Direction) (p: Point) (i: int) =
        match dir with
        | Horizontal -> { p with X = p.X + i }
        | Vertical   -> { p with Y = p.Y + i }
    Seq.init this.Length (add this.Direction this.Position)

member this.IsFinished =
    this.Points
    |> Seq.forAll (fun x -> Seq.contains x this.Hits)

member this.HitTest (pos: Point) =
    this.Points
    |> Seq.contains pos

This means that IsFinished doesn't rely on Hits not having erroneous duplicates or somehow containing points that don't lie on the ship's area. The HitTest function is greatly simplified too. I'd actually take this a step further and move the add function out to the Point type, where it logically resides (see my earlier answer).

---

type Player(name : string, ships : Ship list, enemyHits : Point list) = 
  member this.Name = name
  member this.Ships = ships
  member this.EnemyHits = enemyHits;
  member this.IsFinished = (this.Ships |> List.tryFind (fun sh -> sh.IsFinished = false) ).IsNone

Again I would prefer to implement Player using a record, where you get the Copy functionality for free. The IsFinished function could be implemented a little simpler:

this.Ships |> List.forAll (fun s -> s.IsFinished)

---

  member this.Print() =
    let shipHits = this.Ships |> List.collect (fun sh -> sh.Hits)

    printf "y\x"
    for x in 1 .. boardSize do
      printf " %d " x
    printfn ""

    for y in 1 .. boardSize do
      for x in 0 .. boardSize do
        let hit = shipHits |> List.tryFind (fun pt -> pt.X = x && pt.Y = y)
        if x = 0 && y > 0 then
          printf "%3d" y
        elif hit.IsSome then
          printf " X "
        else 
          let pt = this.EnemyHits |> List.tryFind (fun pt -> pt.X = x && pt.Y = y)
          if pt.IsSome then
            printf " 0 "
          else
            printf "   ";
      Console.WriteLine()

As a purely presentational function I'd make this separate from the Player class. It's less important in a toy app like this but it's good practice, e.g. you may want to render to OpenGL rather than print to the console.

I'd also suggest removing the string-building logic from the console-printing logic. You can then pass the visual representation around, e.g. you may want to send it over a network or render it on several console windows. You could use pattern matching here too, rather than if/else. You're also doing list searches in nested loops which is a fairly inefficient way to do things, particularly as the game goes on and more hits are recorded. Much better would be to convert Point to a record type, then store in a Set for O(1) lookup:

  member this.Print() =
    let shipHits = this.Ships |> Seq.collect (fun sh -> sh.Hits) |> Set.ofSeq

    seq {
        for y in 0 .. boardSize do
            for x in 0 .. boardSize do
                yield
                    match x, y with
                    | 0, 0 -> "y\x"
                    | 0, i -> sprintf "%2d " i
                    | i, 0 -> sprintf " %d " i
                    | x, y when shipHits |> Set.contains { X = x; Y = y } -> " X "
                    | x, y when this.EnemyHits |> List.contains { X = x; Y = y } - > " 0 "
                    | _ -> "   "
            yield "\n"
    }
    |> String.concat ""
    |> Console.Write

---

  static member Copy(player : Player, hittedShip : Ship option, hit : Point) =
    if  hittedShip.IsSome then
      let shipList = Ship.Copy(hittedShip.Value, hit) :: player.Ships |> List.where (fun sh -> sh <> hittedShip.Value)
      Player(player.Name, shipList, player.EnemyHits)
    else
      Player(player.Name, player.Ships, hit :: player.EnemyHits)

I think this function has the wrong name - you're not just copying the player, you're recording a hit, so I'd rename it to something more appropriate. The implementation is a bit complicated, and I think part of that is because you're storing hits in two places: in Player.EnemyHits, and Ship.Hits. Perhaps hits belong to the board/player, not the ship? Then you wouldn't need to test whether there was a hit when modifying the game state.

---

let getUserTarget() =
  let rec gut() = 
    Console.Write("Enter target [x,y]:")
    let input = Console.ReadLine().Split(',')
    if input.Length <> 2 then
      gut()
    else
      let mutable x = 0
      let mutable y = 0
      if Int32.TryParse(input.[0], &x) && Int32.TryParse(input.[1], &y) then
        if x < 1 || x > boardSize || y < 1 || y > boardSize then
          gut()
        else
          Point(x, y)
      else
        gut()
  gut()

You don't need the auxilliary function because it has the same signature as getUserTarget - so just make that recursive. You can also avoid mutable variables by using the overload of TryParse which returns an Option:

let rec getUserTarget() =
    Console.Write("Enter target [x,y]:")
    let input = Console.ReadLine().Split(',')
    if input.Length <> 2 then
        getUserTarget()
    else
        match Int32.TryParse(input.[0]), Int32.TryParse(input.[1]) with
        | Some x, Some y when x > 0 && x <= boardSize && y > 0 && y <= boardSize -> Point(x, y)
        | _ -> getUserTarget()

You could "simplify" this even further using a regular expression - something like ^([\d]+),([\d]+)$ - but you know what they say about regular expressions...

---

// The "main" function which is repeatedly called by Seq.unfold below in main
let getState (state : GameState) =  
  if state.IsFinished then
    printBoard state
    None
  else
    printBoard state
    let target = getUserTarget()
    let hittest = state.OtherPlayer.Ships |> List.tryFind (fun ship -> ship.HitTest target)
    if hittest.IsSome then
      Some((state, GameState(state.CurrentPlayer, Player.Copy(state.OtherPlayer, hittest, target))))
    else
      Some((state, GameState(Player.Copy(state.OtherPlayer, None, target), state.CurrentPlayer)))

This function is a little confusing. I'd recommend extracting the hittest function to the Player type and rethinking the Player.Copy function (see comments above)

---

General comments

• In FP it's idiomatic to separate functions from instances, so that they can be easily curried. In practice this usually just means making members static and providing the instance as the final argument. For example, the Player.IsFinished function mentioned above could be implemented as player.Ships |> List.forAll Ship.IsFinished.

• You aren't making much use of pattern matching which is a very powerful technique in functional programming.

• Explore the collection modules to see if there is an appropriate function for what you're trying to do, rather than using tryFind etc