current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite
2

So I'm doing a basic MVC layout for a pretty basic game that I am making. The game requires the user to move up/down/left/right via buttons on the GUI. Since I'm using an MVC layout and my buttons are in a different class than the ActionListeners, I was wondering what the best way to add the action listeners are?

Method 1:

View Class ActionListener method:

public void addMovementListeners(ActionListener u, ActionListener d, ActionListener l, ActionListener r){
    moveUp.addActionListener(u);
    moveDown.addActionListener(d);
    moveLeft.addActionListener(l);
    moveRight.addActionListener(r);
}

Control Class add ActionListener method:

private void addListeners(){
    viewGUI.addMovementListeners(new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            buttonPressed = 1;
        }
    },
    new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            buttonPressed = 2;
        }
    },
    new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            buttonPressed = 3;
        }
    },
    new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            buttonPressed = 4;
        }
    });
}

Method 2:

View Class ActionListener method:

public void addMovementListeners(ActionListener a){
    moveUp.addActionListener(a);
    moveDown.addActionListener(a);
    moveLeft.addActionListener(a);
    moveRight.addActionListener(a);
}

public JButton[] getButtons(){
    JButton[] temp = new JButton[4];
    temp[0] = moveUp;
    temp[1] = moveDown;
    temp[2] = moveLeft;
    temp[3] = moveRight;

    return temp;
}

Control Class add ActionListener method:

JButton[] movementButtons;

private void addListeners(){
    movementButtons = viewGUI.getButtons();
    viewGUI.addMovementListeners(new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            if(e.getSource() == movementButtons[0]){
                buttonPressed = 1;
            }else if(e.getSource() == movementButtons[1]){
                buttonPressed = 2;
            }else if(e.getSource() == movementButtons[2]){
                buttonPressed = 3;
            }else if(e.getSource() == movementButtons[3]){
                buttonPressed = 4;
            }
        }
    });
}

Which method is better? Is there another method that is even better than these two? Let me know! Trying to get this MVC thing down :). (Any other suggestions welcome as well)!

share|improve this question
add comment

1 Answer

up vote 2 down vote accepted

Method #1 feels like the better approach, because it doesn't require your view to expose its buttons, but the four-times ActionListener method seems odd. Both methods have some trouble with magic numbers.

Introduce an enum for directions. It may feel like overkill at the moment, but it'll take care of your direction-to-action mapping. It will also help shape some of your options.

Then consider supplying Action instead of ActionListener. This will also make it easier to enable/disable the movement buttons in case something blocks your way (like going off-field), without your view having to expose its buttons.

When you combine these two, you get something like the following:

View:
    void setAction(Direction dir, Action action);

Model:
    void move(Direction dir);
    /** Returns directions possible to move to at this moment. */
    Set<Direction> getPossibleDirections();

enum Direction { UP, RIGHT, DOWN, LEFT; }

class Controller {
  Map<Direction, MoveAction> actions;

  public void setView(View view) {
    actions = new EnumMap<>(Direction.class);
    for ( final Direction direction : Direction.values() ) {
      final MoveAction action = new MoveAction(direction, direction.toString());
      actions.put(direction, action);
      view.setAction(direction, action);
    }
  }

  void refreshState() {
    final Set<Direction> dirs = model.getPossibleDirections();
    for ( final MoveAction action : actions.values() ) {
      action.setEnabled(dirs.contains(action.direction));
    }
  }

  class MoveAction extends AbstractAction {
    final Direction direction;

    MoveAction(Direction direction, String name) {
      super(name);
      this.direction = direction;
    }

    public void actionPerformed(ActionEvent evt) {
      model.move(direction);
      refreshState(); // or have model firePropertyChange
    }
  }
}
share|improve this answer
    
So I understand where you're going with the enums and the whole setActions and setView thing, but after that you kind of lost me. It seems like you are doing the calculation for if whether the player can move or not inside the controller class. Shouldn't this be handled inside of the model? In my current model class I have a movePlayer() method that checks to see if the desired location (based off of direction) is a) a free space and b) inside the bounds. If it is, it moves the player, if not, it executes nothing else. Is this incorrect thinking? –  user3053240 May 4 at 23:15
    
The controller doesn't calculate what directions to go: it asks the model through getPossibleDirections(), and then enables/disables actions according to that. getPossibleDirections() figures out what's legal to do. Is that clearer? –  JvR May 5 at 0:17
    
Yep, that makes sense! Thank you for the help! –  user3053240 May 5 at 0:22
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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