Replacing C# Switch Statement into a More Readable/Intuitive Format

Question

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

Answer 1

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