Interacting with GUI class and Main class

Usually when I make mock-up programs (like this one), I look for things I can improve on in case the situation happens again.

Today I thought I'd brush up on basic OOP (I understand the concept of OOP, just haven't messed around with it for a bit and wanted to freshen my memory). So I decided to make a little game that just creates 3 monsters on a 10x10 plane and 1 player (you), you are able to move your player in any x/y direction. My program works but I can't help but feel that I'm doing something incorrectly.

So the basic layout of my program was to have 5 classes. A GUI class that shows the game and gives you directional buttons for movement control, a class that creates the monsters, a class that creates the players, a class that creates the 10x10 board and keeps track of monster/player locations, and of course a main class that creates all the objects and has the main game loop and whatnot.

I was having a bit of a hard time interacting with my main class and my GUI class. What I ended up doing was doing a while loop in my main class and waiting until the player presses the start button, and once the player presses it (via action listener) the GUI class sets a public variable (running) from false to true, and I am able to act accordingly once the variable is changed.

HERE is where I feel like I am doing something wrong: At first my while loop would not terminate unless I printed out to the console. I Googled the issue and apparently people have said that it's some sort of issue with threading or "active polling", which I did not understand. I went to my program and added a small 10ms thread sleep in my while loops and everything started working great.

My question to you guys is, why do my while loops only terminate when adding a thread sleep? And is there a better way of interacting with a GUI class and a main class?

Sorry for the giant wall of text but I like to be thorough when explaining a situation!

TL;DR: Am I interacting correctly with my GUI cla

I have very little experience with Swing, so forgive me if I cannot answer your specific question (though Stack Overflow might be a better place for that). Still, one thing I always see with Swing programs is the use of SwingUtilities.invokeLater, so maybe that's why you're having troubles. Take a look at how this example starts a Swing project.

I'm sure more experienced people can comment on your overall design and the MVC pattern than I could, so I'll limit myself to a line-by-line review here.

The main method

You are doing far too much here. With a few exceptions, the main method should just get your class up and going. I would like to see something like a Game class which takes the board, players (as a list), and monsters (as a list) in its constructor. Maybe the GUI too, I'm not sure. Like I said, I'm terrible at MVC practices.

The while loop in the main method should also be moved into this Game glass, which controls the main game loop. There's a lot of things you can find on Google about main game loops that are worth reading.

int x, y;
        x = playerOne.getX();
        y = playerOne.getY();

can be simplified to

int x = playerOne.getX();
        int y = playerOne.getY();

Alternatively, the whole input section could be simplified to

while(gui.running){
  if (gui.buttonPress == -1)
    continue;
  }

  if (gui.buttonPress == 1) {
    playerOne.move(playerOne.getX(), playerOne.getY() - 1);
  } else if (gui.buttonPress == 2) {
    playerOne.move(playerOne.getX(), playerOne.getY() + 1);
  } else if (gui.buttonPress == 3) {
    playerOne.move(playerOne.getX() - 1, playerOne.getY());
  } else if (gui.buttonPress == 4) {
    playerOne.move(playerOne.getX() + 1, playerOne.getY());
  }

  ...

(Actually, not really, because you have some additional lines after all this input stuff, but why are you mixing the input logic with anything else? Put it in its own method.)

There's a lot that can be said about this, so I'll just list them in no particular order:

-
Add spaces in your control statements! Everyone's eyes will thank you! Compare

if(gui.buttonPress == 1){

to

if (gui.buttonPress == 1) {

-
There's no point in storing the player's coordinates in these less descriptive variables. They're only going to be accessed once because of the if statement structure.

-
As a tip, this is my enum that I use for handling direction in my programs:

public enum Direction {
  LEFT(-1, 0), RIGHT(1, 0), UP(0, 1), DOWN(0, -1);

  private final int x;

  private final int y;

  Direction(final int x, final int y) {
    this.x = x;
    this.y = y;
  }

  public int getX() {
    return x;
  }

  public int getY() {
    return y;
  }
}

With this, you could create a map of your button input values to directions, and simply do the following:

Direction direction = DIRECTION_MAP.get(gui.buttonPress); // your new direction map
if (direction != null) {
    playerOne.move(playerOne.getX() + direction.getX(), playerOne.getY() + direction.getY();
}

The most egregious part so far, though, is your abuse of magic numbers. What does it mean when buttonPress = 1? Also, why can I even access that variable in the first place?! For more on that, we go to

The ShowGUI class

Purely a stylistic criticism, but I think it ShowGui would be more conventional, even though that that's still a bad class names. This class is named like a method (with a leading verb) which is odd. Why not just Gui or something? Make the class name sound like a noun, not a verb.

Some other comments:

-
running and buttonPress variables should be private with public get methods. In fact, buttonPress shouldn't have one at all. Rather, some controller class should be invoked when this changes (or not, like I said, terrible with MVC). The point is, your game logic shouldn't be asking the view about things; rather, the view should be telling your game logic what happened.

-
There's magic numbers everywhere, which can understand when making a Swing app, though you could look into frame layouts (or whatever they're called) to do positioning for you. As it is, you just have

pane.setLayout(null);

which is a useless call and which I assume was added by a Swing builder.

-
Constants are your friend.

moveUp.addActionListener(new ActionListener() {
    public void actionPerformed(ActionEvent e) {
        running = true;
        buttonPress = 1;
    }
});

Why are you modifying running here? And more importantly, why are you setting buttonPress to an integer? Two Alternatives:

private static final int MOVE_UP = 1;
// later...
buttonPress = MOVE_UP;

or, using the Direction enum above,

buttonPress = Direction.UP;

though that only would work if buttonPress only stored directions.

StackExchange Code Review Q#48868, answer score: 3