DRY principle in ShowGridPositionRange

[Nachiten]

If you want to apply the Dont Repeat Yourself principle on the modification made to ShowGridPositionRange, and avoid copying 20 lines of code to only modify 3, then this is what you can do. Use the same method you already have, and just add an optional bool at the end.

  private void ShowGridPositionRange(GridPosition centerGridPosition, int range, GridVisualType gridVisualType, bool square = false)
    {
        List<GridPosition> gridPositionList = new();
        
        for (int x = -range; x <= range; x++)
        for (int z = -range; z <= range; z++)
        {
            GridPosition possibleGridPosition = centerGridPosition + new GridPosition(x, z);
            
            if (!LevelGrid.Instance.GridPosIsValid(possibleGridPosition)) 
                continue;
            
            int distance = centerGridPosition.Distance(possibleGridPosition);
            
            // Grid pos is too far away (only for not square)
            if (distance > range && !square) // <-- HERE
                continue;
            
            gridPositionList.Add(possibleGridPosition);
        }
        
        ShowGridPositions(gridPositionList, gridVisualType);
    }

The square bool is used on the marked if line. So, only if the distance doesnt qualify for a sphere, and it is a sphere test, then return. If square is true, that check is always false, so it works as if that check is not there.

Hope this helps

[CodeMonkey]

Hmm that’s an interesting modification, but personally I still think two functions makes the code more clear.
What does setting that bool to false do? It is not immediately obvious to me that the opposite of “square” is “circle”, when you come back to that code 6 months from now will you remember that’s what it means?

[bixarrio]

I made posts where I moved the grid position calculations into the actions so that each action can determine its own grid positions

Switch (selectedAction)

I feel like switching on the object type moves logic that belongs in the action into the GridSystemVisual class. Does anyone have a cleaner way that keeps the tile color, attack range, and attack targets logic inside the individual actions? Maybe: BaseAction could expose overridable GetPositionsAndGridVisualTypes() something like: GetPositionsAndGridVisualTypes() returns GridPositionAndGridVisualType[] with whatever logic the action code deems reasonable. where: interface GridPositionAndG…

@CodeMonkey

It is not immediately obvious to me that the opposite of “square” is “circle”

To be fair, it’s also not immediately obvious that ShowGridPositionRange is going to be circular.

[Nachiten]

@bixarrio

To be fair, it’s also not immediately obvious that ShowGridPositionRange is going to be circular.

Thats very true lol. That confusion in my case could be avoided with a simple enum, to make it more expressive. RadiusType.SQUARE, RadiusType.CIRCLE, instead of a bool. That would also allow me to have other shapes if I need to.

The only way I would agree with @CodeMonkey in this case, is if the logic for one radius and the other varies a lot in the future, then I would prefer having two methods.

I guess it comes down to personal preference

[Cat_Hill]

There sure is a lot of common code between ShowGridPositionRange and GetValidGridPosition.

I haven’t refactored mine yet, but my choice would have been to use an enum (over a boolean) Note that we’ve used three different shape types so far - MoveAction is an approximation of a circle*, ShootAction/GrendeAction is a diamond, InteractAction is a Square. It is not hard to imagine other shapes. Imagine a ranged action for example whose Action cannot impact any cell immediately adjacent to the unit but maybe that’s better done with another parameter.

You can also parameterize other stuff as well (e.g. what counts as a valid position if there’s an Enemy or Unit on your own team or an object there). bixarrio’s idea looks interesting too.

*Yes the code operates on a square but you can quickly rule out a bunch of cells on the outer edge with basic math and save yourselves the computational cost of pathfinding on cells you know will fail.

[heckert]

@Nachiten

The only way I would agree with @CodeMonkey in this case, is if the logic for one radius and the other varies a lot in the future, then I would prefer having two methods.

Well, you could easily have both if you did something like this:

class SomeClass
{
  enum SomeTypes { TypeA, TypeB };

  void DoSomethingForA()
  {
    DoSomethingGenerically( SomeTypes.TypeA );
  }

  void DoSomethingForB()
  {
    DoSomethingGenerically( SomeTypes.TypeB );
  }

  void DoSomethingGenerically( SomeTypes thisType )
  {
    // actual code
  }
}