TIGSource Forums

Developer => Technical => Topic started by: tiktokbob on August 05, 2013, 02:59:58 AM


Title: Managing User Interface State (C#/XNA, but should apply cross language)
Post by: tiktokbob on August 05, 2013, 02:59:58 AM
Hello all,

I was hoping I could get some input on how to more gracefully handle GUI interface state.

Currently my current setup is managed by holding a state enum in a variable and different functions are called in the Update(gameTime) method depending on the current state; upon transition to another state a "SetGameStateXXX]" (ie. SetGameStateWaitingInput()) is called which handles the transition to another state. 

All of which is not really the issue; the problem I'm having is the validation of user actions and the state of various GUI components when and the logic against them.  It's all turned into a bit of a mess as I have added more features and I can't help but feel there's got to be a better way of handling it; sample code below.

Any advice or input would be gratefully received!!

Quote

        InGameState inGameState;
        
        /// <summary>
        /// This part of the GUI management is pretty clean and works well
        /// Adding new states or new functionality to a state change is easy
        /// </summary>
        private void SetInGameStateUnitSelected(ICharacter unit)
        {
            dbtnEndTurn.Enabled = true;
            dbtnEndTurn.GreyedOut = false;

            this.selectedUnit = unit;
            this.unitDisplayWindow.UnitDisplay.UnitID = (uint?)selectedUnit.ID;
            floodMovementList = null;

            if (unit.UnitStatus != UnitStatus.Disabled)
            {
                floodRangeList = FloodSearch.RunRangeFloodPath(unit.Coords, unit.MeleeAttackRange, combatMap);
                floodRangeList.Remove(unit.Coords);
            }
            this.inGameState = InGameState.UnitSelected;
        }

        /// <summary>
        /// Here is where the issue is ... a huge mass of if/else which is 
        /// very prone to soaking a lot of time when I want to add new functionality
        /// </summary>
        private void InGameStateUnitSelected()
      {
         if (KeyPressed(Keys.Escape))
         {
            if (displayPath != null)
            {
               displayPath = null;
            }
            else
            {
               SetInGameStateWaitingInput();
            }
         }
         else if (LeftMouseClicked() && cursorMapCoords != null)
         {
            if (selectedUnit.UnitStatus != UnitStatus.Disabled)
            {
               ICharacter clickedUnit = this.combatMap.GetUnitAtCoords(cursorMapCoords);

               if (displayPath != null && displayPath.DestinationCoords() == cursorMapCoords)
               {
                  // invoke movement
                  InvokeMovement();
               }
               // check if state is right for invoking pathfinding
               else if (selectedUnit.TeamID == CombatManager.ActiveTeam.TeamID     // unit is from active team
                       && clickedUnit == null                                      // cell is not occupied
                       && !combatMap.CellBlocked(cursorMapCoords)                  // cell not blocked
                       && floodMovementList.ContainsKey(cursorMapCoords))          // cell is valid to move to
               {
                  // get and store path
                  InvokePathfinding(this.selectedUnit.Coords, this.cursorMapCoords);
               }
               else if (clickedUnit != null)
               {
                  if (selectedUnit.TeamID == CombatManager.ActiveTeam.TeamID
                           && clickedUnit.TeamID != selectedUnit.TeamID)
                  {
                     // display attack options
                     if (selectedUnit.CanAttack && floodRangeList.ContainsKey(clickedUnit.Coords))
                     {
                        ActionResult actionResult = selectedUnit.Attack(clickedUnit);
                        combatTextOutput = actionResult.DisplayString;
                                CombatManager.ActionTaken();
                        // CombatManager.ApplyActionResult(actionResult);
                     }
                  }
                  else if (clickedUnit.ID == selectedUnit.ID)
                  {
                     // TODO: unit action display options
                  }
               }
               else if (clickedUnit != null)
               {
                  // attempt to select unit
                  SetInGameStateUnitSelected(clickedUnit);
               }
            }
         }
         else if (cursorMapCoords != null)
         {
             ICharacter unitHovered = this.combatMap.GetUnitAtCoords(cursorMapCoords);
            if (unitHovered != null 
                        && selectedUnit.CanAttack 
                        && unitHovered.TeamID != selectedUnit.TeamID
                        && floodRangeList.ContainsKey(unitHovered.Coords))
            {
               currentCursor = GUICursor.Attack;
            }
            else
            {
               currentCursor = GUICursor.Default;
            }
         }
      }

Title: Re: Managing User Interface State (C#/XNA, but should apply cross language)
Post by: doihaveto on August 06, 2013, 04:47:52 AM
This big list of if/else-if statements is going to cause problems as you've noticed. One, it's hard to read and understand, and therefore debug; two, it's easy to accidentally introduce side-effects between different parts of the function.

It looks to me like these "if ... else if" conditions are really doing the work of context handling: e.g. mouse click means something else depending on whether some unit is selected or not.

I would pull out different contexts into separate classes, so that you can swap them in and out as the context changes. For example, here's how I structure this:

Code:
  mechanical actions  ->  game actions       ->  action handlers

  (keypress, mouse        (unit de/selected,     (start pathfinding,
  click, ui click)        resource purchased)    check inventory limits)

In my code, there's one set of classes that just convert mechanical actions to game actions. There are different handlers that get swapped in and out depending on context: for example, mouse drag normally moves the game board, but when I'm in "place a building mode", there's a different one that interprets it as moving the building instead. Same with handling mouse clicks differently, etc.

Then there's a second set of classes that handle game actions - these are no longer context sensitive. In my code at least, these tend to be either components on entities (if it was something like unit selected), or global managers (like inventory). They typically get their info via events dispatched by the mechanical handlers, but sometimes get coupled more closely.

Does that help any?

Title: Re: Managing User Interface State (C#/XNA, but should apply cross language)
Post by: nikki on August 06, 2013, 05:59:14 AM
good advice ^

this is the same advice http://www.youtube.com/watch?v=4F72VULWFvc in the form of a 'clean code talk @ google'

1) the basic gist of that talk is: many if-else on state -> you need to use polymorphism and take out the if-elses.

2) and also : many null check ? use a nullObject.

3) and third: you can extract a few useful methods from that code too  

4) and lastly: when you would write tests for your code, you tend to not grow such functions, they are hell to test. tests do make you a better programmer like that. (and they help immensely with refactoring)

Title: Re: Managing User Interface State (C#/XNA, but should apply cross language)
Post by: nikki on August 06, 2013, 10:01:46 AM
I started cleaning up the code as I would do it,
Usually I look for methods to extract first, in this case I only extracted all your conditions,

the code I have now looks like
Quote
        private bool PathIsAvailable() {
            return (displayPath != null && displayPath.DestinationCoords() == cursorMapCoords);
        }
        private bool CouldLookForPath() {
            return (selectedUnit.TeamID == CombatManager.ActiveTeam.TeamID     // unit is from active team
                       && clickedUnit == null                                      // cell is not occupied
                       && !combatMap.CellBlocked(cursorMapCoords)                  // cell not blocked
                       && floodMovementList.ContainsKey(cursorMapCoords));
        }
        private bool AreEnemies() {
            return (selectedUnit.TeamID == CombatManager.ActiveTeam.TeamID && clickedUnit.TeamID != selectedUnit.TeamID);       
        }

        private bool HasAttackOptions() {
            return (selectedUnit.CanAttack && floodRangeList.ContainsKey(clickedUnit.Coords));       
        }

        private bool HoveringOverEnemy() {
            return (unitHovered != null
                        && selectedUnit.CanAttack
                        && unitHovered.TeamID != selectedUnit.TeamID
                        && floodRangeList.ContainsKey(unitHovered.Coords));       
        }

       private void InGameStateUnitSelected()
      {
         if (KeyPressed(Keys.Escape))
         {
            if (displayPath != null)
            {
               displayPath = null;
            }
            else
            {
               SetInGameStateWaitingInput();
            }
         }
         else if (LeftMouseClicked() && cursorMapCoords != null)
         {
            if (selectedUnit.UnitStatus != UnitStatus.Disabled)
            {
               ICharacter clickedUnit = this.combatMap.GetUnitAtCoords(cursorMapCoords);

               if (PathIsAvailable())
               {
                  InvokeMovement();
               }
                else if (CouldLookForPath())
               {
                  InvokePathfinding(this.selectedUnit.Coords, this.cursorMapCoords);
               }
               else if (clickedUnit != null)
               {
                  if (AreEnemies())
                  {
                     if (HasAttackOptions())
                     {
                        ActionResult actionResult = selectedUnit.Attack(clickedUnit);
                        combatTextOutput = actionResult.DisplayString;
                        CombatManager.ActionTaken();
                     }
                  }
                  else if (clickedUnit.ID == selectedUnit.ID)
                  {
                     // TODO: unit action display options
                  }
               }
               else if (clickedUnit != null)
               {
                  SetInGameStateUnitSelected(clickedUnit);
               }
            }
         }
         else if (cursorMapCoords != null)
         {
             ICharacter unitHovered = this.combatMap.GetUnitAtCoords(cursorMapCoords);
            if (HoveringOverEnemy())
            {
               currentCursor = GUICursor.Attack;
            }
            else
            {
               currentCursor = GUICursor.Default;
            }
         }
      }

The refactoring is not done (at all), but after the first cleaning, stuff gets a bit more readable, I think I spotted a bug:
you have this test :
else if (clickedUnit != null)
{
    SetInGameStateUnitSelected(clickedUnit);
}

and just before that branch you have another check for (clickedUnit != null).

so that SetInGameStateUnitSelected() probably never gets fired ?

anyway, I might continue refactoring your code, but it is way more usefull if you learn that sort of thing yourself, the only refactor i have done at this time is http://refactoring.com/catalog/extractMethod.html next up would probably be that if-else-elseif mess that's left.

Title: Re: Managing User Interface State (C#/XNA, but should apply cross language)
Post by: nikki on August 06, 2013, 10:30:19 AM
and just for fun I continued a little
Code:

        private bool PathIsAvailable() {
            return (displayPath != null && displayPath.DestinationCoords() == cursorMapCoords);
        }
        private bool CouldLookForPath() {
            return (selectedUnit.TeamID == CombatManager.ActiveTeam.TeamID     // unit is from active team
                       && clickedUnit == null                                      // cell is not occupied
                       && !combatMap.CellBlocked(cursorMapCoords)                  // cell not blocked
                       && floodMovementList.ContainsKey(cursorMapCoords));
        }
        private bool AreEnemies() {
            return (selectedUnit.TeamID == CombatManager.ActiveTeam.TeamID && clickedUnit.TeamID != selectedUnit.TeamID);       
        }

        private bool HasAttackOptions() {
            return (selectedUnit.CanAttack && floodRangeList.ContainsKey(clickedUnit.Coords));       
        }

        private bool HoveringOverEnemy() {
            return (unitHovered != null
                        && selectedUnit.CanAttack
                        && unitHovered.TeamID != selectedUnit.TeamID
                        && floodRangeList.ContainsKey(unitHovered.Coords));       
        }

        private bool UnitIsEnabled() {
            return (selectedUnit.UnitStatus != UnitStatus.Disabled);       
        }

      private void InGameStateUnitSelected()
      {
         if (KeyPressed(Keys.Escape))
         {
            if (displayPath != null)
            {
               displayPath = null;
            }
            else
            {
               SetInGameStateWaitingInput();
            }
         }
       
        if (cursorMapCoords == null) {return;}
                 
        if (LeftMouseClicked() && UnitIsEnabled())
        {
               ICharacter clickedUnit = this.combatMap.GetUnitAtCoords(cursorMapCoords);

               if (PathIsAvailable())
               {
                  InvokeMovement();
               }
               else if (CouldLookForPath())
               {
                  InvokePathfinding(this.selectedUnit.Coords, this.cursorMapCoords);
               }
               else if (clickedUnit != null)
               {
                  if (AreEnemies() && HasAttackOptions())
                  {
                    ActionResult actionResult = selectedUnit.Attack(clickedUnit);
                    combatTextOutput = actionResult.DisplayString;
                    CombatManager.ActionTaken();
                  }
                  else if (clickedUnit.ID == selectedUnit.ID)
                  {
                     // TODO: unit action display options
                  }
                  else {SetInGameStateUnitSelected(clickedUnit)}
                   
               }
         
         }
         else {
            ICharacter unitHovered = this.combatMap.GetUnitAtCoords(cursorMapCoords);
            currentCursor = HoveringOverEnemy() ? GUICursor.Attack :  GUICursor.Default ;         
         }
      }

your code turned from
Quote
         else if (LeftMouseClicked() && cursorMapCoords != null)
         {
            if (selectedUnit.UnitStatus != UnitStatus.Disabled)
            {
               ICharacter clickedUnit = this.combatMap.GetUnitAtCoords(cursorMapCoords);

               if (displayPath != null && displayPath.DestinationCoords() == cursorMapCoords)
               {
                  // invoke movement
                  InvokeMovement();
               }
               // check if state is right for invoking pathfinding
               else if (selectedUnit.TeamID == CombatManager.ActiveTeam.TeamID     // unit is from active team
                       && clickedUnit == null                                      // cell is not occupied
                       && !combatMap.CellBlocked(cursorMapCoords)                  // cell not blocked
                       && floodMovementList.ContainsKey(cursorMapCoords))          // cell is valid to move to
               {
                  // get and store path
                  InvokePathfinding(this.selectedUnit.Coords, this.cursorMapCoords);
               }
               else if (clickedUnit != null)
               {
                  if (selectedUnit.TeamID == CombatManager.ActiveTeam.TeamID
                           && clickedUnit.TeamID != selectedUnit.TeamID)
                  {
                     // display attack options
                     if (selectedUnit.CanAttack && floodRangeList.ContainsKey(clickedUnit.Coords))
                     {
                        ActionResult actionResult = selectedUnit.Attack(clickedUnit);
                        combatTextOutput = actionResult.DisplayString;
                                CombatManager.ActionTaken();
                        // CombatManager.ApplyActionResult(actionResult);
                     }
                  }
                  else if (clickedUnit.ID == selectedUnit.ID)
                  {
                     // TODO: unit action display options
                  }
               }
               else if (clickedUnit != null)
               {
                  // attempt to select unit
                  SetInGameStateUnitSelected(clickedUnit);
               }
            }
         }
         else if (cursorMapCoords != null)
         {
             ICharacter unitHovered = this.combatMap.GetUnitAtCoords(cursorMapCoords);
            if (unitHovered != null
                        && selectedUnit.CanAttack
                        && unitHovered.TeamID != selectedUnit.TeamID
                        && floodRangeList.ContainsKey(unitHovered.Coords))
            {
               currentCursor = GUICursor.Attack;
            }
            else
            {
               currentCursor = GUICursor.Default;
            }
         }
      }

to
Quote
        if (LeftMouseClicked() && UnitIsEnabled())
        {
               ICharacter clickedUnit = this.combatMap.GetUnitAtCoords(cursorMapCoords);

               if (PathIsAvailable())
               {
                  InvokeMovement();
               }
               else if (CouldLookForPath())
               {
                  InvokePathfinding(this.selectedUnit.Coords, this.cursorMapCoords);
               }
               else if (clickedUnit != null)
               {
                  if (AreEnemies() && HasAttackOptions())
                  {
                    ActionResult actionResult = selectedUnit.Attack(clickedUnit);
                    combatTextOutput = actionResult.DisplayString;
                    CombatManager.ActionTaken();
                  }
                  else if (clickedUnit.ID == selectedUnit.ID)
                  {
                     // TODO: unit action display options
                  }
                  else {SetInGameStateUnitSelected(clickedUnit)}
                   
               }
         
         }
         else {
            ICharacter unitHovered = this.combatMap.GetUnitAtCoords(cursorMapCoords);
            currentCursor = HoveringOverEnemy() ? GUICursor.Attack :  GUICursor.Default ;         
         }
      }

at this point

Title: Re: Managing User Interface State (C#/XNA, but should apply cross language)
Post by: tiktokbob on August 09, 2013, 06:59:34 AM
Nikki and doihaveto,

Thank you very much for your time and input; I've not had much time this week to look at my code due to an office move at work, but I started refactoring looking considerably better and more maintainable.  Your posts have been exceptionally helpful in achieving this!

I had thought about TDD for this project but as it'll be transferred to Unity3D once I've got the game rules framework (including AI & such) in place and I'm happy with them, and I couldn't find anything to nicely support Unity3D TDD, I ended up not using TDD.  This may have been somewhat of a mistake however.  

Quote
so that SetInGameStateUnitSelected() probably never gets fired ?

This would be correct!  A perfect example of why the code was a mess!


Title: Re: Managing User Interface State (C#/XNA, but should apply cross language)
Post by: nikki on August 09, 2013, 12:01:13 PM
glad to be of help

Title: Re: Managing User Interface State (C#/XNA, but should apply cross language)
Post by: doihaveto on August 09, 2013, 11:49:05 PM
Awesome! Glad it was of some use :)


Powered by SMF 1.1.19 | SMF © 2013, Simple Machines