Battleship Game

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 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)

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,

StackExchange Code Review Q#141359, answer score: 3