2
\$\begingroup\$

Recently I had to add 2 different sets of logic for moving my player around the board. One lets the player pick a location and move him there while the other uses a dice. I'm reusing code for both cases and would like to "merge" them into one thing. Any help and suggestions would be greatly appreciated.

void Update()
{
    #region Target Game Mdoe
    if (gameManager.gameMode == GameMode.Target)
    {
        if (currentTile != targetTile && time != 0 && !waitingForCard && isTurn)
        {
            movementProgress += movementSpeed * Time.deltaTime;
            nextSquare = FindNextSquare();
            ChangeCharDirection();


            while (nextSquare != null && movementProgress >= 1.0f)
            {
                movementProgress -= 1.0f;
                travelledTiles.Push(currentTile);
                TriggerTilePassBy();
                currentTile = nextSquare;
                if (currentTile != targetTile)
                {

                    nextSquare = FindNextSquare();

                    TriggerTilePassBy();
                    if (currentTile.tyleType != TyleType.IntersectionTyle)
                    {
                        time--;
                        gameManager.world_ui_Controller.UpdateUI(id);
                    }
                }
                if (currentTile != targetTile && time == 0)
                {
                    hasToSkipTurn = true;
                    Transform[] playerSpots = GetPlayerSpots();
                    StartCoroutine(LerpToPlayerSpot(playerSpots[id].position, Constants.GetInPosTime));
                    HandleTyleType(currentTile);
                    targetTile = currentTile;
                }
                else if (currentTile == targetTile)
                {
                    HandleTyleType(currentTile);
                    // nextSquare = FindNextSquare();
                    nextSquare = null;
                    currDir = PlayerStatus.Still;
                    SetAnimatorBools();
                }
            }
            if (nextSquare != null && targetTile != null)
            {
                transform.position = Vector3.Lerp(currentTile.transform.position, nextSquare.transform.position, movementProgress);
                currDir = PlayerStatus.Moving;
                SetAnimatorBools();
            }
            else
            {
                movementProgress = 0.0f;
                // gameManager.camController.EnableCameraDrag();
                Transform[] playerSpots = GetPlayerSpots();
                StartCoroutine(LerpToPlayerSpot(playerSpots[id].position, Constants.GetInPosTime));
            }
        }
        else if (time == 0)
        {

        }
    }
    #endregion
    #region Dice Game Mode
    if (gameManager.gameMode == GameMode.Dice)
    {
        if (numSpacesToMove != 0 && isTurn && !atIntersection)
        {
            movementProgress += movementSpeed * Time.deltaTime;
            nextSquare = FindNextSquare();
            ChangeCharDirection();
            while (nextSquare != null && numSpacesToMove > 0 && movementProgress >= 1.0f)
            {
                movementProgress -= 1.0f;
                if (numSpacesToMove < 0)
                {
                    currentTile = travelledTiles.Pop();
                    numSpacesToMove++;
                }
                else
                {
                    travelledTiles.Push(currentTile);
                    TriggerTilePassBy();
                    currentTile = nextSquare;
                    numSpacesToMove--;
                }
                if (numSpacesToMove == 0 && !atIntersection)
                {
                    HandleTyleType(currentTile);
                    // nextSquare = FindNextSquare();
                    nextSquare = null;
                    currDir = PlayerStatus.Still;
                    SetAnimatorBools();
                }
                if(currentTile.tyleType == TyleType.IntersectionTyle)
                {
                    atIntersection = true;
                }

            }

        }
        if (atIntersection && numSpacesToMove != 0)
        {
            //enable direction buttons here. Maybe! Or do nothing and wait for user input
            Debug.Log("Waiting for user to press direction button");
        }
        if (nextSquare != null)
        {
            transform.position = Vector3.Lerp(currentTile.transform.position, nextSquare.transform.position, movementProgress);
            currDir = PlayerStatus.Moving;
            SetAnimatorBools();
        }
        else
        {
            movementProgress = 0;
            Transform[] playerSpots = GetPlayerSpots();
            StartCoroutine(LerpToPlayerSpot(playerSpots[id].position, Constants.GetInPosTime));
        }
    }
    #endregion

}
\$\endgroup\$
  • \$\begingroup\$ By reusing you mean copy&paste ;-] let's see what can be done... \$\endgroup\$ – t3chb0t Jun 28 '16 at 7:41
  • \$\begingroup\$ yes that is what I mean. I scraped this yesterday just to have it work but now my whole class feels ugly because of it. Thank you for the help ;) \$\endgroup\$ – Uri Popov Jun 28 '16 at 7:42
3
\$\begingroup\$

I can't find any common denominator in both ifs. They are similar but too different. You could however refactor them into separate methods:

void Update()
{
    switch (gameManager.gameMode) 
    {
        case GameMode.Target:
            UpdateTarget();
            break;
        case GameMode.Dice:
            UpdateTarget();
            break;
        // the compiler will complain without it
        default:
            throw new ArgumentOutOfRangeException(
                "Invalid game mode: " + gameManager.gameMode);
    }    
}

void UpdateTarget() 
{
    // ...
}

void UpdateDice() 
{ 
    // ...
}

I'll go to hell for this ;-) but often when I have if/else if conditions like this one:

if (currentTile != targetTile && time == 0)
{
    hasToSkipTurn = true;
    Transform[] playerSpots = GetPlayerSpots();
    StartCoroutine(LerpToPlayerSpot(playerSpots[id].position, Constants.GetInPosTime));
    HandleTyleType(currentTile);
    targetTile = currentTile;
}
else if (currentTile == targetTile)
{
    HandleTyleType(currentTile);
    // nextSquare = FindNextSquare();
    nextSquare = null;
    currDir = PlayerStatus.Still;
    SetAnimatorBools();
}

I construct them in this way to avoid comments and give each condition a clear name:

var canSomething = currentTile != targetTile && time == 0; 
if (canSomething)
{
    hasToSkipTurn = true;
    Transform[] playerSpots = GetPlayerSpots();
    StartCoroutine(LerpToPlayerSpot(playerSpots[id].position, Constants.GetInPosTime));
    HandleTyleType(currentTile);
    targetTile = currentTile;
}
else
{
    var canSomethingElse = currentTile == targetTile; 
    if (canSomethingElse)
    {
        HandleTyleType(currentTile);
        // nextSquare = FindNextSquare();
        nextSquare = null;
        currDir = PlayerStatus.Still;
        SetAnimatorBools();
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks I was stuck with trying to find a common thing as well. Nice refactoring idea I already have a private method region in the same class so the logic goes there. \$\endgroup\$ – Uri Popov Jun 28 '16 at 8:05
  • \$\begingroup\$ I'm glad. ;-) as far as other aspects of your code are concerned there is not much to comment on. Good variable names, everything is clear to me. However you may consider to better describe the conditions inside ifs and use a helper variable like var canMove = currentTile != targetTile && time != 0 && !waitingForCard && isTurn or whatever name makes sense for them. \$\endgroup\$ – t3chb0t Jun 28 '16 at 8:08
  • \$\begingroup\$ I honestly did not know you can do that. Thanks again I will look into making the conditions clearer. \$\endgroup\$ – Uri Popov Jun 28 '16 at 8:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.