0

So 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, what is active polling? Why is it bad? How/why/where was this going on in my program? And finally if there's 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 class and my main class? If not what is the proper way to do it?

My main class:

public class MainGame {

public static void main(String[] args) throws InterruptedException{
    ShowGUI gui = new ShowGUI();

    while(!gui.running){
        Thread.sleep(10);
    }

    Board gameBoard = new Board();
    gui.setLabelText(gameBoard.getBoard());

    //Add Player
    Player playerOne = new Player(1000, "Player 1");

    //Add monsters
    Monster monstMatt = new Monster(1000, "Matt");
    Monster monstJon = new Monster(1000, "Jon");
    Monster monstAbaad = new Monster(1000, "Abaad");

    while(gui.running){
        Thread.sleep(10);

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

        if(gui.buttonPress != -1){
            if(gui.buttonPress == 1){
                playerOne.move(x, --y);
            }else if(gui.buttonPress == 2){
                playerOne.move(x, ++y);
            }else if(gui.buttonPress == 3){
                playerOne.move(--x, y);
            }else if(gui.buttonPress == 4){
                playerOne.move(++x, y);
            }
            gui.buttonPress = -1;
            gui.setLabelText(gameBoard.getBoard());
        }
    }


}

}

My GUI Class:

public class ShowGUI{

private JTextArea board;
private JButton moveUp;
private JButton moveDown;
private JButton moveLeft;
private JButton moveRight;

public boolean running = false;
public int buttonPress = -1;

public ShowGUI(){
    System.out.println("GUI Successfully Loaded");
    createAndShow();
}

private void createAndShow(){
    JFrame mainFrame = new JFrame("Bad Game");

    addComponents(mainFrame.getContentPane());

    mainFrame.setSize(500, 400);
    mainFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    mainFrame.setLocationRelativeTo(null);
    mainFrame.setResizable(false);
    mainFrame.setVisible(true);
}

private void addComponents(Container pane){
    pane.setLayout(null);

    board = new JTextArea(1, JLabel.CENTER);
    moveUp = new JButton("Up");
    moveDown = new JButton("Down");
    moveLeft = new JButton("Left");
    moveRight = new JButton("Right");

    moveUp.setBounds(185, 225, 130, 35);
    moveLeft.setBounds(115, 280, 130, 35);
    moveRight.setBounds(255, 280, 130, 35);
    moveDown.setBounds(185, 335, 130, 35);

    board.setEditable(false);
    board.setBounds(115, 30, 270, 145);
    board.setFont(new Font("Consolas", Font.BOLD, 12));

    addActionListeners();

    pane.add(board);
    pane.add(moveUp);
    pane.add(moveRight);
    pane.add(moveLeft);
    pane.add(moveDown);
}

private void addActionListeners(){
    moveUp.addActionListener(new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            running = true;
            buttonPress = 1;
        }
    });
    moveDown.addActionListener(new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            buttonPress = 2;
        }
    });
    moveLeft.addActionListener(new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            buttonPress = 3;
        }
    });
    moveRight.addActionListener(new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            buttonPress = 4;
        }
    });

}

public void setLabelText(char[][] boardToShow){
    board.setText(" ");
    for(int i = 0; i < boardToShow.length; i++){
        for(int j = 0; j < boardToShow[i].length; j++){
            board.append(boardToShow[i][j] + "   ");
        }
        board.append("\n ");
    }
}

}

If you require my Board/Monster/Player classes I can post them, but I don't think the problem is with those classes.

user3053240
  • 33
  • 1
  • 6
  • 2
    This question would fit better on [codereview](http://codereview.stackexchange.com/) – Absurd-Mind May 03 '14 at 15:06
  • And remember to use more descriptive titles – keyser May 03 '14 at 15:10
  • @keyser Sorry about that! I've edited the title in my new question to be a bit more specific. Edit: Here is the link if anyone would like to answer it: http://codereview.stackexchange.com/questions/48868/im-probably-doing-this-incorrectly-is-there-a-better-way-interacting-with-gu – user3053240 May 03 '14 at 15:13
  • 1
    You're also making the beginner mistake of extending `JFrame`, then creating a `JFrame`. It's one or the other, not both. Not that it matters functionally, but it tells a lot about the writer. – Kayaman May 03 '14 at 15:13
  • @Kayaman Ah you're right. I haven't messed around with swing much either. It makes sense as to why you don't need both, thanks! This little program was to help me a bit with my swing as well. – user3053240 May 03 '14 at 15:17
  • A few comments: (1) You might want to print "GUI Successfully Loaded" *after* is was "Successfully Loaded" with `createAndShow()`. (2) Instead of `\n` for new lines use `System.getProperty("line.separator")` or another system-independent approach. (3) Consider a `switch` statement on `gui.buttonPress`, they are more readable and maintainable than `if-else`. (4) Consider using `pack()` instead of `setSize()` for your main frame. (5) Consider using 1 `ActionListener` for all your movement operations - have a variable passed to it in the constructor indicating the direction. – user1803551 May 03 '14 at 18:02

1 Answers1

0

Active polling (a.k.a. busy waiting) is when a program checks over and over whether some condition is true, and since we're talking about computers, this is usually done many times a second. It's bad because it means the process is constantly using up CPU to check for this condition, and, even worse, because it uses up so much of the CPU, it can prevent the condition from being able to become true in the first place (this is what is happening in your case).

I'll try to explain this with a metaphor, where your Main class (specifically your while(gui.running) loop) is the boss, and the GUI class is the employee. Imagine the boss comes to his employee every second and asks "Have you done what I asked you to do?". By doing so every second, not only is the boss wasting his time, but is actually preventing his employee from being able to do what he was asked to do.

That is exactly what is happening between your Main and GUI class. The value of buttonPress can not change when the while loop keeps running, and that's why sleeping (and printing to console, because that requires an IO operation which also blocks the thread for a while) makes it work, because the while loop stops executing for a short amount of time, giving your program the chance to change the value of buttonPress.

The solution

In order to solve this, let's go back to the boss/employee metaphor. Instead of checking every second, the boss should say, "When you are done doing this, come and tell me". That way he doesn't need to keep checking on the employee and the employee has time to do the task. Similarly, your GUI class should call a method in your Main class when the actionListeners are fired. Ultimately, as other people pointed out, your structure needs quite a bit of work and clean up, I would recommend you look into using the Model-View-Controller pattern.

However I will propose a solution which will work for the setup you currently have. Your Main class should look something like:

public class Main {

     private Player playerOne;
     private Monster monstMatt;
     private Monster monstJon;
     private Monster monstAbaad;
     private ShowGUI gui;
     private Board gameBoard;

     public Main() {

         //Add Player
         playerOne = new Player(1000, "Player 1");

         //Add monsters
         monstMatt = new Monster(1000, "Matt");
         monstJon = new Monster(1000, "Jon");
         monstAbaad = new Monster(1000, "Abaad");        

         gui = new ShowGUI(this);

         gameBoard = new Board();
         gui.setLabelText(gameBoard.getBoard());

     }

     public movePlayerUp() {
         movePlayer(0, -1);
     }

     public movePlayerDown() {
         movePlayer(0, 1);
     }

     public movePlayerLeft() {
         movePlayer(-1, 0);
     }

     public movePlayerRight() {
         movePlayer(1, 0);
     }

     private movePlayer(x, y) {
         playerOne.move(playerOne.getX() + x, playerOne.getY() + y);
     }

}

And the GUI class:

public class ShowGUI {

     private Main main;

     public ShowGui(Main main) {
          this.main = main;
          createAndShow();
          System.out.println("GUI Successfully Loaded");
     }

     private void addActionListeners(){
          moveUp.addActionListener(new ActionListener() {
               public void actionPerformed(ActionEvent e) {
                    main.movePlayerUp();
               }
          });
          moveDown.addActionListener(new ActionListener() {
               public void actionPerformed(ActionEvent e) {
                    main.movePlayerDown();
               }
          });
          moveLeft.addActionListener(new ActionListener() {
               public void actionPerformed(ActionEvent e) {
                    main.movePlayerLeft();
               }
          });
          moveRight.addActionListener(new ActionListener() {
               public void actionPerformed(ActionEvent e) {
                    main.movePlayerRight();
               }
          });
     }         

     /* All the other methods you have */
     ...

}

So instead of using flags (buttonPress=2), use methods which will be called when a certain action occurs. Again, this is not a perfect long-term solution, but it has the right gist and is the sort of pattern you should follow.

Community
  • 1
  • 1
Illidanek
  • 996
  • 1
  • 18
  • 32