I was desiring to clean up my code a bit for a battleship program that I was creating. There is a 2D array gameboard that holds char values pertaining to each of the ships found on the board, i.e. a battleship on the gameboard would be represented as 'BBBB'.

The program already runs, but the portion which checks whether the user's selected coordinate contains a char value that pertains to a ship seems messy, and I do not know of a way to make it look cleaner. Before this, it was implemented as a bunch of if else-if statements, and that didn't really look too clean either. Any help or pointers to guide me in the right direction would be great. Thanks.

            switch (board.Grid[userSelectedRow, userSelectedCol])
            {
                case 'C':
                    carrier_C_HitCounter  ;
                    hitCounter  ;
                    if (carrier_C_HitCounter != shipList[0].Length)
                    {
                        Console.WriteLine("You struck a ship!\n");
                    }
                    else
                        Console.WriteLine("You sunk their carrier!\n");
                    break;
                case 'B':
                    battleship_HitCounter  ;
                    hitCounter  ;
                    if (battleship_HitCounter != shipList[1].Length)
                    {
                        Console.WriteLine("You struck a ship!\n");
                    }
                    else
                        Console.WriteLine("You sunk their battleship!\n");
                    break;
                case 'S':
                    submarine_S_HitCounter  ;
                    hitCounter  ;
                    if (submarine_S_HitCounter != shipList[2].Length)
                    {
                        Console.WriteLine("You struck a ship!");
                    }
                    else
                        Console.WriteLine("You sunk their submarine!");
                    break;
                case 's':
                    submarine_s_HitCounter  ;
                    hitCounter  ;
                    if (submarine_s_HitCounter != shipList[3].Length)
                    {
                        Console.WriteLine("You struck a ship!");
                    }
                    else
                        Console.WriteLine("You sunk their submarine!");
                    break;
                case 'D':
                    destroyer_D_HitCounter  ;
                    hitCounter  ;
                    if (destroyer_D_HitCounter != shipList[4].Length)
                    {
                        Console.WriteLine("You struck a ship!");
                    }
                    else
                        Console.WriteLine("You sunk their destroyer!");
                    break;
                case 'd':
                    destroyer_d_HitCounter  ;
                    hitCounter  ;
                    if (destroyer_d_HitCounter != shipList[5].Length)
                    {
                        Console.WriteLine("You struck a ship!");
                    }
                    else
                        Console.WriteLine("You sunk their destroyer!");
                    break;
                default:
                    Console.WriteLine("No ships were hit.");
                    break;
            }
            // Change the hit location to 'X'
            board.SetChar(userSelectedRow, userSelectedCol, 'X');

CodePudding user response：

The code that is being repeated the most is your check for what ship it is and generating the associated message:

if (carrier_C_HitCounter != shipList[0].Length)
{
     Console.WriteLine("You struck a ship!\n");
}
else
     Console.WriteLine("You sunk their carrier!\n");

You could define an enum and a function to generate the messages like so to clean up the switch statement a bit:

enum Ship
{
    "carrier",
    "battleship",
    "submarine",
    "destroyer"
}


public void getMessage(int number, int counter){
    if (counter != shipList[number].Length)
    {
         Console.WriteLine("You struck a ship!");
    }
    else
         Console.WriteLine("You sunk their "   (Ship)number   " !");
}

Then call it from the switch like:

switch (board.Grid[userSelectedRow, userSelectedCol])
{
case 'C':
    carrier_C_HitCounter  ;
    hitCounter  ;
    getMessage(0,carrier_C_HitCounter);
    break;
case 'B':
    battleship_HitCounter  ;
    hitCounter  ;
    getMessage(1,battleship_HitCounter);
    break;
case 'S':
    submarine_S_HitCounter  ;
    hitCounter  ;
    getMessage(2,submarine_S_HitCounter);
    break;
case 's':
    submarine_s_HitCounter  ;
    hitCounter  ;
    getMessage(2,submarine_s_HitCounter);
    break;
case 'D':
    destroyer_D_HitCounter  ;
    hitCounter  ;
    getMessage(3,destroyer_D_HitCounter);
    break;
case 'd':
    destroyer_d_HitCounter  ;
    hitCounter  ;
    getMessage(3,destroyer_d_HitCounter);
    break;
default:
    Console.WriteLine("No ships were hit.");
    break;
}
// Change the hit location to 'X'
board.SetChar(userSelectedRow, userSelectedCol, 'X');

I'm assuming shipList is global but otherwise modify the function header to pass that in too. The most important thing in making you code look cleaner is to look for large repeating chunks and try to find a way to "reroute" it. Hope this helps Derek :) Welcome to SO

CodePudding user response：

Perhaps you could take it a step further. Instead of characters, use an abstract class:

public abstract class ShipBase
{
    public ShipBase() { }

    public int Length { get; protected set; }
    public int HitCounter { get; protected set; }
    public string Name { get; protected set; }

--  Get message
    public string DamageMessage()
    {
        if (HitCounter == Length)
        {
            return $"You sunk their {Name}";
        }
        return "You struck their ship!";
    }
}

Each subclass changes properties.

public class BattleShip : ShipBase
{
    public BattleShip()
    {
        Length = 4;
        HitCounter = 0;
        Name = "battleship";
    }
}

Now your switch becomes:

    if (board.Grid[userSelectedRow, userSelectedCol] is ShipBase shipHit)
    {
        shipHit.HitCounter  ;
        Console.WriteLine($"{shipHit.DamageMessage()}\n");
        -- So you cannot hit a spot that was already hit
        board.Grid[userSelectedRow, userSelectedCol] = null;
    }
    else
    {
        Console.WriteLine("No ships were hit.");
    }

If you need another type of ship, you just add a class. As long as new ship type follows the blueprint, you do not need to change any code.