2
\$\begingroup\$

I am creating a simple program with multiple buttons. Is this way of checking which button was pressed is ok? Is it better to use switch..case.. statement?

public class KennelGUI extends JFrame implements ActionListener{
    private String filename; // holds the name of the file
    private Kennel kennel; // holds the kennel

    int buttonsNumber = 10;
    private JButton[] buttons = new JButton[buttonsNumber];

    private KennelGUI() {
        super("Kennel with GUI");

        do{
            filename = JOptionPane.showInputDialog("Please enter the filename of kennel information: ");
        }while(filename==null || filename.isEmpty()); //not good

        kennel = new Kennel();

        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setLocation(300,100);
        setSize(200,500); 
        setVisible(true);
        setLayout(new FlowLayout());

        buttons[0] = new JButton("Add dog");
        buttons[1] = new JButton("Add cat");
        buttons[2] = new JButton("Add monkey");
        buttons[3] = new JButton("Set capacity");
        buttons[4] = new JButton("Print dogs with bones");
        buttons[5] = new JButton("Print all animals");
        buttons[6] = new JButton("Remove dog");
        buttons[7] = new JButton("Set kennel name");
        buttons[8] = new JButton("Search for pet");
        buttons[9] = new JButton("Quit");

        for(int i=0;i<buttonsNumber;i++)
        {
            add(buttons[i]);
            buttons[i].addActionListener(this);
            buttons[i].setPreferredSize(new Dimension(190, 30));
        }
    }

    /*
     * initialise() method runs from the main and reads from a file
     */
    private void initialise() {
        JOptionPane.showMessageDialog(null, "Using file " + filename);

        try(FileReader fr = new FileReader(filename);
            BufferedReader br = new BufferedReader(fr);
            Scanner infile = new Scanner(br)){
            Animal newAnimal = null;

            String kennelName = infile.nextLine();
            kennel.setName(kennelName);

            int kennelSize = infile.nextInt();
            infile.nextLine();
            kennel.setCapacity(kennelSize);

            int numPets = infile.nextInt();
            infile.nextLine();

            for(int i=0; i < numPets; i++){
                String type = infile.nextLine();
                switch(type)
                {
                    case "Dog":
                        newAnimal = new Dog();
                        break;
                    case "Cat":
                        newAnimal = new Cat();
                        break;
                    case "Monkey":
                        newAnimal = new Monkey();
                        break;
                    default:
                        throw new IOException("Type problem");//GUI?
                }
                newAnimal.load(infile);             
                kennel.addAnimal(newAnimal);
            }

        } catch (FileNotFoundException e) {
            JOptionPane.showMessageDialog(null,"The file: " + filename + " does not exist.\nAssuming first use and an empty file.\n" +
                               "If this is not the first use then have you \naccidentally deleted the file?","Warning",JOptionPane.WARNING_MESSAGE);
        } catch (IOException e) {
            JOptionPane.showMessageDialog(null,"An unexpected error occurred when trying to open the file " + filename,"Warning",JOptionPane.WARNING_MESSAGE);
        }
    }


        @Override
        public void actionPerformed(ActionEvent e) {

            switch(e.getSource()){
              case buttons[0]:
                admitDog();
                break;
              case buttons[1]:
                admitCat();
                break;
              case buttons[2]:
                admitDog();
                break;
              case buttons[3]:
                setKennelCapacity();
                break;
              case buttons[4]:
                printDogsWithBones();
                break;
              case buttons[5]:
                printAll();
                break;
              case buttons[6]:
                removePet();
                break;
              case buttons[7]:
                changeKennelName();
                break;
              case buttons[8]:
                searchForPet();
                break;
              case buttons[9]:  
                save();
                System.exit(0);
                break;
            }

        }
}
\$\endgroup\$
  • \$\begingroup\$ Ok, sorry I am new. Is it better now? I pasted fixed code (changed if..else into switch..case) \$\endgroup\$ – siema Apr 5 '15 at 10:40
  • \$\begingroup\$ Thank you. Now we can see that some of the buttons are related to each other (add dog, add cat, add monkey), which may affect the advice. \$\endgroup\$ – 200_success Apr 5 '15 at 10:41
  • \$\begingroup\$ I had similar question some time ago, however in my case, I used one big inner class, not method as you. I got advice to split actionPerformed into several classes. You can check for more here: codereview.stackexchange.com/questions/84887/… \$\endgroup\$ – m.cekiera Apr 16 '15 at 11:52
2
\$\begingroup\$

I'm really not sure if this switch statement works at all. You're comparing objects with it, so your compiler should state that switch only takes integer (or since java 7 string) values.

Normally you wouldn't use e.getSource() and use e.getActionCommand() instead. This allows you to decouple the button objects from their behaviour through using string constants as their action commands.

If you don't set an actionCommand at creation time, you may set it later at the buttons through button[i].setActionCommand(...). If you don't set an action command at all, its default value is the text you give to the button for displaying.

First you should put your possible actions into string constants:

public final static String ADD_DOG_ACTION = "ADD DOG";

then use these constants to set the action command at your buttons:

button[0].setActionCommand(ADD_DOG_ACTION);

and last use these constants in your switch:

switch(e.getActionCommand()){
    case ADD_DOG_ACTION: doSomething();break;
    case ...
    case default: System.err.println("Unknown Action " + e.getActionCommand());

Once you have your program up and running, you should have a look at the Model-View-Controller pattern. It's relatively easy to implement and helps to decouple your logic (Kennel) from your GUI.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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