Coding Button functions in separate classes

Question

I've been trying to separate my button functions from my main GUI class The code works but as an programming beginner i want to hear if i'm doing things the 'proper' way. Point out what am i doing properly and what not. Thanks in advance.

Frame class:

GuiFrame() {
    setTitle("Aplikacija (bez imena) v1.0.1");
    setSize(800, 600);
    setLocationRelativeTo(null);
    setContentPane(new GuiPanel());
    setDefaultCloseOperation(EXIT_ON_CLOSE);


    setResizable(false);
    setLocationRelativeTo(null);
    setVisible(true);

}

GUI main Panel class :

JLabel title = new JLabel("Aplikacija za dijagnozu bolesti");
JLabel diagnose = new JLabel("Preliminarna dijagnoza : ");

private final String[] listS1 = {"-Odaberi-", "Akutni", "Hronicni"};
JComboBox comboS1 = new JComboBox();
private int counterS1 = 0;

private final String[] listS2 = {"-Odaberi-", "Abdomen", "Udovi", "Glava"};
JComboBox comboS2 = new JComboBox();
private int counterS2 = 0;

private final ComboBoxModel[] models = new ComboBoxModel[5];

private final JComboBox comboS3 = new JComboBox();

JLabel simptom1 = new JLabel("Vrsta bola koju osecate : ");
JLabel simptom2 = new JLabel("U kom delu tela osecate taj bol :");
JLabel simptom3 = new JLabel("Vas bol osecate u (vidi sliku) : ");

public final ImageIcon rep = new ImageIcon(getClass().getResource("/images/rsz_flag_of_the_red_cross.png"));
public final ImageIcon rep2 = new ImageIcon(getClass().getResource("/images/rsz_s13.jpg"));
JLabel picture01 = new JLabel(rep);
JLabel picture02 = new JLabel(rep2);

JTextField diagnoseField = new JTextField();

Font font = new Font("Times new Roman", Font.BOLD, 14);

JButton reset = new JButton();
JButton calculate = new JButton();

public GuiPanel() {

    setLayout(null);

    models[0] = new DefaultComboBoxModel(new String[]{"-Odaberi-"});
    models[1] = new DefaultComboBoxModel(new String[]{"-Odaberi-", "1", "2", "3", "4", "5", "6", "7", "8", "9"});

    title.setBounds(90, 40, 200, 100);
    title.setFont(font);
    add(title);

    picture01.setBounds(400, 40, 350, 400);
    add(picture01);

    picture02.setBounds(400, 40, 350, 400);
    picture02.setVisible(false);
    add(picture02);

    simptom1.setBounds(10, 100, 200, 100);
    add(simptom1);

    simptom2.setBounds(10, 200, 200, 100);
    add(simptom2);

    simptom3.setBounds(10, 300, 200, 100);
    add(simptom3);

    comboS1.setBounds(245, 135, 90, 30);
    for (int i = 0; i < 3; i++) {
        comboS1.addItem(listS1[counterS1++]);
    }
    add(comboS1);

    comboS2.setBounds(245, 235, 90, 30);
    for (int i = 0; i < 4; i++) {
        comboS2.addItem(listS2[counterS2++]);
    }
    add(comboS2);

    comboS3.setBounds(245, 335, 90, 30);
    comboS3.setModel(models[0]);
    comboS3.disable();
    add(comboS3);

    diagnose.setBounds(10, 420, 200, 100);
    add(diagnose);

    diagnoseField.setBounds(10, 490, 350, 30);
    diagnoseField.setEditable(false);
    add(diagnoseField);

    reset.setText("Ponovo");
    reset.setBounds(640, 470, 110, 50);
    add(reset);

    calculate.setText("Dijagnoza");
    calculate.setBounds(400, 470, 110, 50);
    calculate.addActionListener(comboS1);
    add(calculate);

    CalculateButton c = new CalculateButton(calculate, comboS1, comboS2, comboS3, diagnoseField);
    AgainButton a = new AgainButton(reset, comboS1, comboS2, comboS3, diagnoseField, picture01);

    again();
    whenAkutniAbdomen();
    //diagnose();

}

The Calculate button class

public class CalculateButton implements ActionListener {

    private final JComboBox comboS1;
    private final JComboBox comboS2;
    private final JComboBox comboS3;
    private final JTextField diagnoseField;

    public CalculateButton(JButton calculate, JComboBox comboS1, JComboBox comboS2, JComboBox comboS3, JTextField diagnoseField) {
        this.comboS1 = comboS1;
        this.comboS2 = comboS2;
        this.comboS3 = comboS3;
        this.diagnoseField = diagnoseField;
        calculate.addActionListener(this);
    }

    @Override
    public void actionPerformed(ActionEvent e) {

        if (comboS1.getSelectedIndex() == 1 && comboS2.getSelectedIndex() == 1 && comboS3.getSelectedIndex() == 1) {
            diagnoseField.setText("Rak na pluca");
        }

    }

}

The second AgainButton class is the same as the calculate class. Only the code which is in "" is in my native language and should be viewed as plain strings, everything else should be understandable.

Answer 1

Use lazy initialization

You currently separate the component creation process by immediately instantiating the component at the variable declaration and configuring the component elsewhere.

Component creation and configuration should be encapsulated in lazy getters like this:

private JLabel picture01;

private JLabel getPicture01() {
    if (picture01 == null) {
        picture01 = new JLabel(getRep()/* another lazy getter */);
        picture01.setBounds(400, 40, 350, 400);
    }
    return picture01;
}

If you do not use the variable anywhere else you may consider to only have a factory method:

private JLabel createPicture01() {
    JLabel picture01 = new JLabel(getRep()/* another lazy getter */);
    picture01.setBounds(400, 40, 350, 400);
    return picture01;
}

There are at least two main points this method addresses.

1. Having the the whole construction at one place
2. If you use the lazy getter only you do not have to care about the creation time. You are more flexible when rearranging the components

I recommend this because of the the points mentioned AND the following argument:

I saw several GUI editors that generated code like this. As the code must be EASILY parseable by the GUI editor this seems to be an easy to parse structure. And what are we developers doing when we read code? We are parsing it!

Visibility modifiers

Try to have the least visibility of any declared variable. Several components are declared in package scope:

JLabel simptom1;

Make them private:

private JLabel simptom1;

Constants

You access some resources like this:

getClass().getResource("/images/rsz_flag_of_the_red_cross.png");

Make them a "public static final" constant. Resources should be loaded only once and not every time an object is instantiated.

There are other variables that may be declared as constants like "listS1" or "listS2". If they are not meant to be changed then they are also constant candidates.

An common convention is to write constants UPPER_CASE.

Listener registration

The listener registration should be within the configuration part of the component (see lazy getter) and not in the listener itself.

private JButton getCalculateButton() {
    if (calculateButton == null) {
        calculateButton = new JButton();
        calculateButton.setText("Dijagnoza");
        calculateButton.setBounds(400, 470, 110, 50);
        calculateButton.addActionListener(comboS1);
        calculateButton.addActionListener(new CalculateButtonListener(getComboS1(), getComboS2(), getComboS3(), getDiagnoseField()));
    }
    return calculateButton;
}

Shutdown strategy

Currently you define following to close your application:

setDefaultCloseOperation(EXIT_ON_CLOSE);

I do not recommend this because any pending or working thread will be aborted. If this is your intention then you have no well-defined shutdown as the threads may be in an arbitrary state (writing to harddisk, communicating with remote services etc.).

Also you will hide programming errors if some threads have corrupt shutdown mechanisms.

So inform all threads to shutdown if you want to exit the program. After all you should define following shutdown strategy for Swing:

setDefaultCloseOperation(DISPOSE_ON_CLOSE);

Separation of concerns / Missing model layer

As you are asking the GUI components directly and setting the GUI components in the ActionListener you have no layer separation. UI classes know the ActionListener and the ActionListener knows UI classes. It doesn't matter that they are different.

if (comboS1.getSelectedIndex() == 1 && comboS2.getSelectedIndex() == 1 && comboS3.getSelectedIndex() == 1) {
    diagnoseField.setText("Rak na pluca");
}

You are totally missing a model layer. I don't know if you have the intention to introduce one. But setting the text of "diagnoseField" directly is across country.

I'd expect the ActionListener call the model so it will change. The change will be populated to "diagnoseField" through another listener mechanism so the ActionListener isn't aware of what has to be changed in the UI.

Answer 2

Looks good in my eyes.

I usually tend to follow the MVC architecture where you separate the model, view and controller and you have pretty much followed this principle so I would say that this is well written and the proper way in this situation.

It's easy to read and understand (except your native language which seems to be polish? bosnian?)

Answer 3

Inconsistent modefiers in GUI main Panel

Inside this class, you have put many different field, some with, some without modifiers, this makes it look messy. By assigning the same modifiers for every field your code will look cleaner.

private final JLabel title = new JLabel("Aplikacija za dijagnozu bolesti");
private final JLabel diagnose = new JLabel("Preliminarna dijagnoza : ");

private final String[] listS1 = {"-Odaberi-", "Akutni", "Hronicni"};
private final JComboBox comboS1 = new JComboBox();
private       int counterS1 = 0;

private final String[] listS2 = {"-Odaberi-", "Abdomen", "Udovi", "Glava"};
private final JComboBox comboS2 = new JComboBox();
private       int counterS2 = 0;

private final ComboBoxModel[] models = new ComboBoxModel[5];
private final JComboBox comboS3 = new JComboBox();

private final JLabel simptom1 = new JLabel("Vrsta bola koju osecate : ");
private final JLabel simptom2 = new JLabel("U kom delu tela osecate taj bol :");
private final JLabel simptom3 = new JLabel("Vas bol osecate u (vidi sliku) : ");

public final  ImageIcon rep = new ImageIcon(getClass().getResource("/images/rsz_flag_of_the_red_cross.png"));
public final  ImageIcon rep2 = new ImageIcon(getClass().getResource("/images/rsz_s13.jpg"));

private final JLabel picture01 = new JLabel(rep);
private final JLabel picture02 = new JLabel(rep2);

private final JTextField diagnoseField = new JTextField();

private final Font font = new Font("Times new Roman", Font.BOLD, 14);

private final JButton reset = new JButton();
private final JButton calculate = new JButton();

Finally, grouping them by function or usage will make the purpose cleaner.

You are using absolute positioning

While I can agree that absolute positioning of the elements is the easiest to learn, you should use a layout manager to lay the elements out, this allows you to support resizing.