# Battleship Model - follow-up

This is the revised code from my prior question. Same requests as before! Something I'm doing new here is using the XmlEncoder extension methods to handle serialization and de-serialization.

XmlEncoder.cs

using System;
using System.IO;
using System.Text;
using System.Xml.Serialization;

namespace Battleship
{
public static class XmlEncoder
{
public static string XmlSerializeToString(this object objectInstance)
{
var serializer = new XmlSerializer(objectInstance.GetType());
StringBuilder sb = new StringBuilder();

using (TextWriter writer = new StringWriter(sb))
{
serializer.Serialize(writer, objectInstance);
}

return sb.ToString();
}

public static T XmlDeserializeFromString<T>(this string objectData)
{
return (T)XmlDeserializeFromString(objectData, typeof(T));
}

public static object XmlDeserializeFromString(this string objectData, Type type)
{
var serializer = new XmlSerializer(type);
object result;

{
}

return result;
}
}
}


GridSquare.cs

namespace Battleship
{
public enum SquareType
{
Foggy,
Water,
Undamaged,
Damaged,
Sunk
}

public class GridSquare
{
public SquareType Type { get; set; }
public int ShipIndex { get; set; }

public GridSquare()
{
Type = SquareType.Foggy;
ShipIndex = -1;
}
}
}


Grid.cs

namespace Battleship
{
public class Grid
{
public GridSquare[,] Squares { get; private set; }
public int Size { get; private set; }

public Grid(int gridSize)
{
Size = gridSize;
Squares = new GridSquare[gridSize, gridSize];

for (int x = 0; x < gridSize; ++x)
{
for (int y = 0; y < gridSize; ++y)
{
Squares[x, y] = new GridSquare();
}
}
}
}
}


Ship.cs

namespace Battleship
{
// the classic rules (patrol boats aren't fit for warfare)
public enum ShipType
{
Carrier = 5,
Battleship = 4,
Cruiser = 3,
Submarine = 3,
Destroyer = 2
}

public enum Orientation
{
South = 0,
East = 1,
North = 2,
West = 3
}

internal static class OrientationExtension
{
public static Orientation GetOrientation(this int n)
{
switch (n)
{
case 1:
return Orientation.East;
case 2:
return Orientation.North;
case 3:
return Orientation.West;
default:
return Orientation.South;
}
}
}

public class Ship
{
public ShipType ShipType { get; set; }
public Orientation Orientation { get; set; }
public int Health { get; set; }

public Ship()
{
ShipType = ShipType.Destroyer;
Repair();
}

public int GetLength()
{
return (int)ShipType;
}

public void Repair()
{
Health = GetLength();
}

public bool Sunk()
{
return Health == 0;
}

public void Hit()
{
if (Health > 0)
{
--Health;
}
}
}
}


Player.cs

using System;
using System.Collections.Generic;
using System.Linq;

namespace Battleship
{
public class Player
{
public Grid Grid { get; set; }
public List<Ship> Ships { get; set; }

protected static Random rand = new Random();

public Player(int gridSize)
{
Grid = new Grid(gridSize);
Ships = new List<Ship>();

foreach (ShipType type in Enum.GetValues(typeof(ShipType)))
{
{
ShipType = type
});
}

PlaceShips();
}

private void PlaceShips()
{
for (int shipIndex = 0; shipIndex < Ships.Count; ++shipIndex)
{
int x = rand.Next(Grid.Size);
int y = rand.Next(Grid.Size);
GridSquare sea = Grid.Squares[x, y];
Ship ship = Ships[shipIndex];
List<Orientation> validOrientations = new List<Orientation>();

// calculate valid orientations
for (int i = 0; i < 4; ++i)
{
Orientation o = i.GetOrientation();
bool done = false;

try
{
for (int j = 1; j < ship.GetLength() && !done; ++j)
{
switch (o)
{
case Orientation.South:
if (Grid.Squares[x, y - j].ShipIndex != -1)
{
done = true;
}
break;
case Orientation.East:
if (Grid.Squares[x - j, y].ShipIndex != -1)
{
done = true;
}
break;
case Orientation.North:
if (Grid.Squares[x, y + j].ShipIndex != -1)
{
done = true;
}
break;
case Orientation.West:
if (Grid.Squares[x + j, y].ShipIndex != -1)
{
done = true;
}
break;
}

if (j == Grid.Size - 1)
{
}
}
}
catch (Exception err)
{
if (err is IndexOutOfRangeException)
{
Console.WriteLine(ship.ShipType + " fell off the edge of the world while heading " + o + "!");
}
}
}

if (!validOrientations.Any())
{
throw new Exception("The current grid cannot fit all of the ships!");
}

sea.Type = SquareType.Undamaged;
sea.ShipIndex = shipIndex;
ship.Orientation = validOrientations[rand.Next(validOrientations.Count)];

// pick an orientation at random and layout the ship
for (int i = 1; i < ship.GetLength(); ++i)
{
switch (ship.Orientation)
{
case Orientation.South:
Grid.Squares[x, y - i].ShipIndex = shipIndex;
break;
case Orientation.East:
Grid.Squares[x - i, y].ShipIndex = shipIndex;
break;
case Orientation.North:
Grid.Squares[x, y + i].ShipIndex = shipIndex;
break;
case Orientation.West:
Grid.Squares[x + i, y].ShipIndex = shipIndex;
break;
}
}
}
}

public void Attack(int x, int y)
{
GridSquare sea = Grid.Squares[x, y];

switch (sea.Type)
{
case SquareType.Foggy: // miss
sea.Type = SquareType.Water;
break;
case SquareType.Undamaged:
Ship ship = Ships[sea.ShipIndex];

ship.Hit();

if (ship.Sunk())
{
sea.Type = SquareType.Sunk;

for (int i = 1; i < ship.GetLength(); ++i)
{
switch (ship.Orientation)
{
case Orientation.South:
Grid.Squares[x, y - i].Type = SquareType.Sunk;
break;
case Orientation.East:
Grid.Squares[x - i, y].Type = SquareType.Sunk;
break;
case Orientation.North:
Grid.Squares[x, y + i].Type = SquareType.Sunk;
break;
case Orientation.West:
Grid.Squares[x + i, y].Type = SquareType.Sunk;
break;
}
}
}
else
{
sea.Type = SquareType.Damaged;
}

break;
default:
throw new InvalidOperationException("You cannot change that square type!");
}
}

public bool GameOver()
{
return Ships.All(ship => ship.Sunk());
}
}
}

• You might want to study this implementation. It's VBA, but it's full-blown OOP (MVC) and you'll find that the model classes' (AIPlayer, PlayerGrid, Ship) responsilities are very clear-cut - GridSize has no business in a Player class, for example, and then the strategy an AI player uses to place its ships or pick an attack position can be abstracted and injected, too. – Mathieu Guindon Oct 4 '18 at 23:16
• Looks good! I reverted my prior change to adhere to OOP standards. – T145 Oct 4 '18 at 23:26

### Inconsistent use of var

I love var, I use it pretty much everywhere. IMO not using it makes C# read like Java, and that can't be good. But that's just my opinion, and given a code base that doesn't use it, I'll conform to the style in place and keep its [non-]usage consistent - because whether you choose to use it or not, what matters is consistency. With that in mind, this is rather disturbing:

        var serializer = new XmlSerializer(objectInstance.GetType());
StringBuilder sb = new StringBuilder();


Syntactically, the two statements are essentially the same. Yet one uses var, and the other doesn't, for no apparent reason. Since that's the only place var is used, I'd change var serializer to XmlSerializer serializer and move on.

### ++thing | thing++

Seeing ++i is disturbing. It makes you stop and think "wait what's the reason for pre-[in|de]crementing here?", for no reason at all, because there's no reason at all to do ++i in a for loop, besides coding style perhaps (makes no difference because the increment's returned value isn't used anyway). That's why some will see it as a subjective nitpick, but IMO if typing for and hitting TAB in Visual Studio makes a default auto-template that does i++, then it's reasonable to consider i++ in a for loop as the least surprising "style"... now that said you're consistent about using prefix in situations that make no difference, so I'll give you that. But I would have no difficulty believing postfix is the prevalent and expected "style".

If you're using ++i because of some misguided micro-performance optimization concern, I encourage you to read Eric Lippert's excellent SO answer describing how prefix and postfix works.

public static Orientation GetOrientation(this int n)


I don't think this is a good extension method to have. Especially since Orientation is an enum, which already readily casts to/from int... and I would expect the default case to throw, not return some arbitrary value. But switch is typically seen switching on enums, not on integers. It's ...weird, and likely not needed at all.

The name is wrong, too. ToOrientation would be more appropriate. "Get" sounds like you're fetching something; "To" is commonly used to convey a type conversion that involves copying or transforming the data: ToArray, ToString. Then again, (Orientation)value is much more straightforward.

public enum SquareType
{
Foggy,
Water,
Undamaged,
Damaged,
Sunk
}


The terminology isn't completely unambiguous: the enums could use a <summary> xml-doc comment to enhance IntelliSense when using them.

/// <summary>The state of a single grid position.</summary>
public enum SquareType
{
/// <summary>Unknown state.</summary>
Foggy,
/// <summary>Grid position was previously attacked, and revealed water.</summary>
Water,
/// <summary>Grid position contains a ship.</summary>
Undamaged,
/// <summary>Grid position was previously attacked, and revealed a ship.</summary>
Damaged,
/// <summary>Grid position was previously attacked, and the ship was sunken.</summary>
Sunk
}


Unless I'm misunderstanding what each value stands for, I think the enum is mixing concerns a bit: Sunk isn't a grid position state, it's a ship state: as far as the game grid is concerned, whether the ship there is sunken or just hit makes no difference: there's a red pin at that position. I believe more descriptive names could be used, that would make the xml-doc less necessary:

public enum GridState
{
Unknown,
ShipPosition,
PreviousMiss,
PreviousHit,
}


The Orientation enum is fine, but IMO could be simplified to Horizontal and Vertical - whether it's pointing left or pointing right doesn't make any difference, unless your UI wants to make that difference... but then again, game-wise, it changes nothing and I'd concentrate on getting a solid model first, and then augment it with bells & whistles like game state serialization and 4-directional ships.

### GridSquare

IMO the class is superfluous, and knows way too many things about its world: a grid cell should be just that: a grid cell, i.e. a SquareType (/GridState) value. It's not its job to know or care about what specific ship might occupy that space, even less to know that ships are indexed, and that a specific index corresponds to a particular ship. A GridSquare class would be something I'd write at the UI layer, with information such as PixelSize, BorderColor, some Click, DoubleClick, and Hover handlers: it has nothing to do in the model!

### Grid

I find the grid class rather anemic, ..and IMO the grid size should be constant: Battleship is played on a 10x10 grid in every variant that involves 5 ships with sizes 5-4-3-3-2. If you want a parameterized grid size, then you'll want configurable fleet setups, ...but at this point it's scope-creep: keep it stupid simple (KISS).

Your grid is essentially a 2D array of grid state values: it might as well be a 2D array of grid state values.

### Ship

public class Ship
{
public ShipType ShipType { get; set; }
public Orientation Orientation { get; set; }
public int Health { get; set; }


Why would a ship's ShipType or Orientation need to ever change after it's created? These should be get-only, immutable values assigned in the constructor. And if Health is meant to be assigned through Hit and Repair methods, then the setter shouldn't be public.

public Ship(ShipType shipType, Orientation orientation)
{
ShipType = shipType;
Orientation = orientation;
}

public ShipType ShipType { get; }
public Orientation Orientation { get; }
public int Health { get; private set; }


I like the simplicity of modeling ship damage as "hit points", however the "health" alone doesn't tell us anything about the state of the ship (i.e. where is it hit?), which means our API needs to get the state of the ship... from not-the-ship.

private bool[] _state;


With an internal array of bool values, we can track the ship's health and know where it's hit, in one place. Then the Health can be get-only as well:

public int Health => _state.Count(e => e); // counts the 'true' elements in _state


### Player

Again, mutability is problematic here; Grid and Ships should have private setters, if at all:

    public Grid Grid { get; set; }
public List<Ship> Ships { get; set; }


I agree with a player owning a grid, but as I said above I would have made the grid own the ships.

• I wasn't too sure about how powerful the XmlSerialization stuff is, and left some fields that would be privately set completely public so nothing would go wrong. Every single Microsoft example uses public instance variables. – T145 Oct 5 '18 at 13:19
• So I wonder, if you add the _state array to Ship to store information about which part is hit, do you still save the state in Grid? The undamaged/damaged/sunk distinction is made in gridstate. If you're removing the ship index information from the grid, you would have to move the hit methods and their logic to the ships. – JAD Oct 5 '18 at 13:19
• @JAD IMO a ship needs to know its position, direction, and hit-state; the grid needs to know where the ships are, independently of ship state - so you can have an unknown grid state while having an enemy ship at that position. Now I might have a design bias there, for I recently made a battleship game (see github link in comments under the OP), but yes IMO ship and grid state need to be distinct. – Mathieu Guindon Oct 5 '18 at 13:30
• @MathieuGuindon I agree with you here, but does the grid need to know whether that ship was damaged or not? Also, if I understand correctly, you say the grid should own the ships, so then the grid also should own the logic for handling hits right? – JAD Oct 5 '18 at 13:33
• @Mr.Vix XML serialization does require public getters and setters. Hence I'd have a dedicated model for serialization - wouldn't let a persistence concern drive the design... like I said I'd make a solid working model first, then think of adding features like game state serialization. – Mathieu Guindon Oct 5 '18 at 13:34