C++ object not saving variable values

Question

I am a beginner C++ programmer, self-taught. I am developing a simple roguelike game as a learning project. The problem I'm having is in the code used to determine if the player is able to move in the specified direction. When the player wants to move one square east for example, the following code is run, where int a and b are the intended square's X and Y coordinates:

bool Location::canMove(int a, int b)
{
    for (int i = 0; i < Decorations.size(); i++)
    {
        if (Decorations[i].getPosX() == b && Decorations[i].getPosY() == a)
        {
            Match = true;
            Matched = Decorations[i];
            break;
        } else
        {
            Match = false;
        }
    }

    if (Match == true)
    {
        if (Matched.Clips() == true)
        {
            Blocked = true;
            return false;
        } else
        {
            Blocked = false;
            return true;
        }
    } else
    {

        switch(Map[a][b])
        {
            case TILE_VOID:
            case TILE_FLOOR:
            Blocked = false;
            return true;
            break;
        case TILE_WALL:
            Blocked = true;
            return false;
            break;
        }
    }
}

Essentially what it does is sees if the player's current location has any Decorative objects at those X and Y coordinates, like tables, pillars etc. If it finds it has one, it then runs a check to see if that object allows a player to pass through it or not with the Clips() function and returns true or false to determine if the player can move there. If no decoration object is found, it then checks the base tile of that location, if it's void or floor then the player can move, if it's a wall then they can't.

This piece of code works fine as far as I can tell (the player isn't able to move through walls or solid objects). However, in the main loop there is another function that is called if the player is told they cannot move, which informs the player why:

string Location::blockMsg()
{
    blockmsg = "You bump into the ";
    if (Blocked == false)
    {
        blockmsg = blockmsg + "wall";
    } else
    {
        blockmsg = blockmsg + Matched.getName();
    }
    return blockmsg;

}

This is used to inform the player why they couldn't move in that direction. If they bumped into a table it should say "You bump into the table", for example.

The problem I'm having is that it doesn't look like the bool Matched or Blocked, or the decoration Matched are being saved once the first piece of code finishes. As such, the logic always acts as though Blocked == false, even if the player has bumped into a decoration that blocks their movement. The result is that if a player is blocked by something, it will always say "You bump into the wall" even if it's something else.

The only assumption I can make is that for some reason, when assigning Blocked = true in the first function, it's not retained for whatever reason for the next function to use it.

Can anyone point out where I've gone wrong? I always assumed that once a Location's Match, Matched and Blocked values are set during the canMove() function, they should retain those same values for blockMsg() to use, but apparently not.

Answer 1

I have made several key assumptions, like the "a" parameter in your original code was equivalent to "x" map coordinate and that "b" param was = "y" map coordinate. I changed the parameter names to give more clarity on the operations of your code.

I am also assuming that the "Locations" class has access to the "Decorations" object and the "Map" 2d array. If "Locations" class does not have access to the Decorations object or the Map array, then that could be causing some issues.

Also if Blocked, Matched and Match are data members of the Location class, their values should persist beyond the function call, but just to make sure its not some weird compiler error, try including the explicit "this->" ("this" is a pointer to the parent object or process) reference to all of your instance variables. Doing this will ensure the operations of your code are saving the data in some sort of persistent variable instead of just disappearing after execution. Note, I have made that change in the code sample below:

bool Location::canMove(int x, int y) //change param names for clarity sake
{
  //I recommend turning the "bool Location::Match" into a "int Location::Match"
  //  I also recommend this for the "bool Location::Blocked" variable
  //  This way you can have a flag that says, yes, no, error, etc.
  //  For illustration purposes I commented out old and inserted new with that integer assumption
  //  Key for the new "Location::Match" and "Location::Blocked" values:
  //    -1  =  error
  //    0   =  false
  //    1   =  true

  //Initialize Match to an error state, that way if the for loop below fails to
  //  execute, your application will flag that failure
  this->Match = -1;

  for (int i = 0; i < Decorations.size(); i++)
  {
      // did you mean to have your values criss-crossed in the "if" statement below?
      if (Decorations[i].getPosX() == y && Decorations[i].getPosY() == x) 
      {
          this->Match = 1;
          this->Matched = Decorations[i];
          break;
      } else
      {
          this->Match = 0;
      }
  }

  if (this->Match == 1) //Match found!
  {
      if (this->Matched.Clips() == true) 
      {
          this->Blocked = 1;
          return false;
      } else 
      {
          this->Blocked = 0;
          return true;
      } 
  } else if (this->Match == 0) // no match found
  {

      switch(Map[x][y]) //changed to reflect new parameter names
      {
      case TILE_VOID:
      case TILE_FLOOR:
          this->Blocked = 0; //false
          return true;
          //break; // break is unreachable, due to the return above, therefore not needed
      case TILE_WALL:
          this->Blocked = 1; //true
          return false;
          //break; //see case above
      default:
          //Always include a default case
          //  log it, report it, write it to file, print to screen, debug, etc.
          this->Blocked = -1; //error
          break;
      }
  } else if (this->Match == -1) // error case
  {
      //Yet another error case...
      //  By adding separate, discrete error cases you can track down the
      //  source of your errors more efficiently
      this->Blocked = -1; // error
      return false;
  }

}

After making those small changes to allow Blocked and Match to have more options than boolean flags, you must change the output message function to work with the new values. (yes I realize that if you use Match or Blocked elsewhere they will have to be updated.)

Here is what the output message would look like:

string Location::blockMsg()
{
    blockmsg = "You bump into the ";
    if (this->Blocked == 0)
    {
        blockmsg += "wall";
    } else if(this->Blocked == 1)
    {
        blockmsg += Matched.getName();
    }else if(this->Blocked == -1)
    {
        blockmsg += "ERROR!"; 
    }
    return blockmsg;

}

By making those changes you should be able to more effectively debug your code and be able to figure out what is happening and where you are going wrong. Also if you are able to, use your debugger and flag the "canMove()" method so that when it gets called you can watch the execution go step for step.

Good Luck and have fun!

~Hv6^3