I have code that is responsible to determine move position in a hex grid when the user moves the stick on the gamepad.

cameraAdjustedMovement is a Vector3. _xThreshold and _yThreshold is a float. _isOdd is a bool.

Code is very simple, we just check input from cameraAdjustedMovement and determine where to move. The _isOdd case is basically when where we want to move is a pointy hex, so if you constantly input up, then you will go in a straight line in a hex grid, switching between left and right.

Is there a way to improve it without making it too complicated or is this code as good as it can get?

if (cameraAdjustedMovement.x < -_xThreshold)
{
    if (cameraAdjustedMovement.z > _yThreshold)
    {
        movePosition = ScenarioManager.EAdjacentPosition.ETopLeft;
    }
    else if (cameraAdjustedMovement.z < -_yThreshold)
    {
        movePosition = ScenarioManager.EAdjacentPosition.EBottomLeft;
    }
    else
    {
        movePosition = ScenarioManager.EAdjacentPosition.ELeft;
    }
}
else if (cameraAdjustedMovement.x > _xThreshold)
{
    if (cameraAdjustedMovement.z > _yThreshold)
    {
        movePosition = ScenarioManager.EAdjacentPosition.ETopRight;
    }
    else if (cameraAdjustedMovement.z < -_xThreshold)
    {
        movePosition = ScenarioManager.EAdjacentPosition.EBottomRight;
    }
    else
    {
        movePosition = ScenarioManager.EAdjacentPosition.ERight;
    }
}
else
{
    if (_isOdd)
    {
        if (cameraAdjustedMovement.z > _yThreshold)
        {
            movePosition = ScenarioManager.EAdjacentPosition.ETopRight;
            _isOdd = !_isOdd;
        }
        else if (cameraAdjustedMovement.z < -_yThreshold)
        {
            movePosition = ScenarioManager.EAdjacentPosition.EBottomRight;
            _isOdd = !_isOdd;
        }
    }
    else
    {
        if (cameraAdjustedMovement.z > _yThreshold)
        {
            movePosition = ScenarioManager.EAdjacentPosition.ETopLeft;
            _isOdd = !_isOdd;
        }
        else if (cameraAdjustedMovement.z < -_yThreshold)
        {
            movePosition = ScenarioManager.EAdjacentPosition.EBottomLeft;
            _isOdd = !_isOdd;
        }
    }
}

CodePudding user response：

My main concern with this code is its repetition: you're making the same checks over and over, and performing very similar actions over and over. This gives lots of space for bugs to hide in, and means that any fixes need to be applied in multiple places. Enigmativity appears to have found one such bug in your sample code already.

First let's look at the repeated checks. In your current code you make the check for movement up and down the grid dependant on the movement left and right in the grid. i.e.

if (moves left)
  if (moves up)
    move topleft
  else if (moves down)
    move downleft
  else 
    move left
else if (moves right)
  if(moves up)
...

But these checks don't actually depend on each other, so instead we can just make each check once. Note that I've introduces a couple of new enums to encode the possible directions.

enum HMovement { Left, Right, None }
enum VMovement { Up, Down, None }

HMovement hMovement;
if(cameraAdjustedMovement.x < -_xThreshold)
{
  hMovement = HMovement.Left;
} 
else if(cameraAdjustedMovement.x > _xThreshold)
{
  hMovement = HMovement.Right;
}
else
{
  hMovement = HMovement.None;
}

VMovement vMovement;
if(cameraAdjustedMovement.z > _yThreshold)
{
  vMovement = VMovement.Up;
} 
else if(cameraAdjustedMovement.z < -_yThreshold)
{
  vMovement = VMovement.Down;
}
else
{
  vMovement = VMovement.None;
}

This removes some of the repeated checks, but it adds in a repeated action: inside every case of the if-else tree we're assigning to the same variable. This is somewhere else we could introduce errors. Instead, let's switch to using the conditional expression syntax:

enum HMovement { Left, Right, None }
enum VMovement { Up, Down, None }

var hMovement = 
    cameraAdjustedMovement.x < -_xThreshold ?   HMovement.Left :
    cameraAdjustedMovement.x > _xThreshold ?    HMovement.Right :
                                                HMovement.None;

var vMovement =
    cameraAdjustedMovement.z > _yThreshold ?    VMovement.Up :
    cameraAdjustedMovement.z < -_yThreshold ?   VMovement.Down :
                                                VMovement.None;

Now we have the two directions, we need to combine them to work out the net movement. Given that all we're doing is assigning something again, we're going to learn our lesson from the last step and use expression syntax again (this time with a switch expression).

enum Movement { Left, TopLeft, TopRight, Right, BottomRight, BottomLeft, None}

(var movePosition, isOdd) =
    (hMovement, vMovement, isOdd) switch {
        (HMovement.Left, VMovement.Up, _) => (Movement.TopLeft, isOdd),
        (HMovement.Left, VMovement.Down, _) => (Movement.BottomLeft, isOdd),
        (HMovement.Left, VMovement.None, _) => (Movement.Left, isOdd),

        (HMovement.Right, VMovement.Up, _) => (Movement.TopRight, isOdd),
        (HMovement.Right, VMovement.Down, _) => (Movement.BottomRight, isOdd),
        (HMovement.Right, VMovement.None, _) => (Movement.Right, isOdd),

        (HMovement.None, VMovement.Up, true) => (Movement.TopRight, false),
        (HMovement.None, VMovement.Down, true) => (Movement.BottomRight, false),

        (HMovement.None, VMovement.Up, false) => (Movement.TopLeft, true),
        (HMovement.None, VMovement.Down, false) => (Movement.BottomLeft, true),

        (HMovement.None, VMovement.None, _) => (Movement.None, isOdd)
    }

Note here that, while in your code the case where the user does not move is implicit (you simply do not have the final else case after the if..elseif), here we have to define it. This is an expression, rather than a statement so it must always return the same kind of thing, no matter which branch it took.

CodePudding user response：

You could try using switch statements, IMO it makes the code look way cleaner and more readable. Here is the documentation, should come in handy: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/statements/selection-statements#the-switch-statement.