I honestly couldn't come up with a better title since it's a scenario-based question:

We have a Battleships game, and we want to ask the player for a pair of coordinates to set one of their ships on the grid (more specifically, the starting point and the ending point of the ship). Assuming that the coordinates were properly given and that the ship is in bounds of the grid plane, we only need to check if the ship collides with any other ship currently on the grid plane.

Context: A Grid has a Content property, and it's either ShipContent or EmptyContent.

The CollisionChecker() method loops through the space between the pair of coordinates that were previously given (mind that these can't be diagonal, this is also assumed to be checked prior).

The player wants to put their battleship between A1 and A4. Since the letters are equal, we loop through 1 to 4, simple enough. However, the player could've entered A4 and A1 respective to their order. Both of these scenarios are expected to work as they are logically sound, but they can cause OutOfBound exceptions and/or improper loops if they are not handled accordingly.

Last bit of context, CoordinateLetter is an enum that has the entire English alphabet in it.

static bool CollisionChecker(Grid[][] gridPlane, CoordinateLetter coordinateLetter1, CoordinateLetter coordinateLetter2, int coordinateNumber1, int coordinateNumber2)
    {
        bool lettersEqual = coordinateLetter1 == coordinateLetter2;
        bool cL1Bigger = coordinateLetter1 > coordinateLetter2;
        bool cN1Bigger = coordinateNumber1 > coordinateNumber2;
        if (lettersEqual && cN1Bigger)
            for (int num = coordinateNumber2; num <= coordinateNumber1; num  )
            {
                // Assume that if it collides, it returns false
            }
        else if (lettersEqual && !cN1Bigger)
        {
            for (int num = coordinateNumber1; num <= coordinateNumber2; num  )
            {
                // Assume that if it collides, it returns false
            }
        }
        else if (!lettersEqual && cL1Bigger)
        {
            for (int num = (int)coordinateLetter2; num <= (int)coordinateLetter1; num  )
            {
                // Assume that if it collides, it returns false
            }
        }
        else
        {
            for (int num = (int)coordinateLetter1; num <= (int)coordinateLetter2; num  )
            {
                // Assume that if it collides, it returns false
            }
        }
        return true;
    }

This block of code sends shivers down my spine. I don't like chaining if else's like this. What would be a better way to implement this?

CodePudding user response：

You could make the code easier to follow and maintain, maybe a little more logical in the naming.

But sometimes you do what you have to.

This is just me messing around with it...

    static bool CollisionChecker(Grid[][] gridPlane, CoordinateLetter gridRow1, CoordinateLetter gridRow2, int gridCol1, int gridCol2)
    {
        bool sameRow = (gridRow1 == gridRow2);
        
        if (!(sameRow || (gridCol1 == gridCol2)))
            throw new ArgumentException("Diagonal not allowed!");
        
        if (sameRow)
        {
            int row = (int)gridRow1;
            int start = Math.Min(gridCol1, gridCol2);
            int end   = Math.Max(gridCol1, gridCol2);

            for (int i = start; i <= end; i  )
            {
                //Check gridPlane[row][i]
            }
        }
        else //Same column
        {
            int col = gridCol1;
            int start = Math.Min((int)gridRow1, (int)gridRow2);
            int end   = Math.Max((int)gridRow1, (int)gridRow2);

            for (int i = start; i <= end; i  )
            {
                //Check gridPlane[i][col]
            }
        }

        return true;
    }

CodePudding user response：

What you're really doing here is interpolating between two vectors, so you don't need to handle each of the cases explicitly, you can just use their normalized difference to step through the grid. Let me explain better.

First, let's replace the letters in your coordinate system with numbers (just to make things simpler) so that A1 = (1; 1), B4 = (2; 4), and so on. Imagine that you're trying to go from E1 to A1, or X = (5; 1) to Y = (1; 1). To make that journey in one go, you need to remove 4 units from the first component of X, which can be written mathematically as X (-4; 0) = Y. Rearranging this gives us the formula to find the difference between any two vectors: Y - X = (-4; 0), that is, just subtract your starting position (X) from your target position (Y).

But you don't want to make this journey in one go, you want to do it in small steps to check if another ship is blocking you. Now, since you're in a discrete integer grid (i.e. you're either in position 1 or 2, you can't be in position 1.5), this simplifies things: the longest step you can take while not missing any ships along the way is of length 1. So if the difference between your points is (-4; 0), you want a step of (-1; 0). If the difference is (0; 5), you want a step of (0; 1). This process of taking a vector and reducing it to a length of 1 while keeping its direction is called normalization. Since your vectors will always be axis-aligned (i.e. you won't be walking diagonally), you can cheat and just use Math.sign on each component instead of doing a "full" vector normalization.

Putting it all together gives you something like this (consider this pseudocode, I don't even know if it'll compile):

int diffX = (int)coordinateLetter2 - (int)coordinateLetter1;
int diffY = coordinateNumber2 - coordinateNumber1;

int stepX = Math.sign(diffX);
int stepY = Math.sign(diffY);

CoordinateLetter currentX = coordinateLetter1;
int currentY = coordinateNumber1;
while (currentX != coordinateLetter2 || currentY != coordinateNumber2)
{
    if (HasCollision(currentX, currentY))
        return false;

    currentX  = stepX;
    currentY  = stepY;
}

return true;

I would also recommend creating a struct to hold both coordinates of a position (the letter and the number) so that you can do operations directly on a position rather than on its components. That makes it easier to reason about 2D code.