# 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 < 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]:")
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")
0


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 }

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]:")
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]:")
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)

• 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. – Henrik Hansen 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 – Henrik Hansen 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. – Henrik Hansen 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. – Henrik Hansen 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! – AlexFoxGill Sep 22 '16 at 13:53