Coding Button functions in separate classes

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

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.

• Having the the whole construction at one place

• 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.

StackExchange Code Review Q#119625, answer score: 3