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

Community · Accepted Answer · 2017-04-13 12:40:54Z

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

Point struct vs record: My thought about thats was that I want a Point to behave as a value type, but maybe mixing my understandig of F# with that of C#/C/C++. classes vs records: As a OO programmer I just love classes.. But you have a good point about copying Records and because they are immutable. Using records instead of classes will in fact solve my problem with player states that survives from one game state to the next - if I understand you correctly. I'll keeep that in mind. — user73941, Sep 19 '16 at 15:29
Ship.IsFinished,.HitTest etc. I tried to minimize the use of data (for instance by avoiding to create a board-matrix to keep track of ships an bombs), so I find my approach ok, but I see what you mean, and I'll have to check for duplicate hits. Your Player.IsFinished is much smoother than my rather clumsy approach — user73941, Sep 19 '16 at 15:30
Player.Print() you are right: I should be separated from the model in a real world as you would do in a model-view-pattern I have a lot to learn about match ... with, it is rather powerful. For instance your use of two variables and the with-statement is new to me and makes the code much more readable. — user73941, Sep 19 '16 at 15:30
About the search in the point lists while printing: it is obviously not a good design, but for a relative small number of items in the lists it is acceptable on a modern computer. It is in fact only necessary when writing to the console due to the rigid behavoir of the console. My goal was to minimize the use of data. The penalty for that is often more work as it is here. — user73941, Sep 19 '16 at 15:31
@HenrikHansen Although I decided not to highlight the use of unfold, I believe that sequences should be idempotent/free of side effects. This includes IO. Since the game state requires external input in order to unfold (and also prints the state in each "step" of the sequence"), I would prefer a simple recursive loop here. Not much state-of-the-art about that though! — Alex, Sep 22 '16 at 13:53

Stack Exchange Network

current community

your communities

more stack exchange communities

Battleship Game

1 Answer 1

General comments

Your Answer

Linked

Hot Network Questions

Battleship Game

1 Answer 1

General comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related

Hot Network Questions